-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/message signing #6
base: main
Are you sure you want to change the base?
Changes from all commits
b9cdc1f
1c9b3e2
0d8e1e3
df484a3
0361381
af023c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
package io.approov.service.okhttp; | ||
|
||
import java.util.ArrayList; | ||
import okhttp3.Request; | ||
|
||
/* Add the following interfaces */ | ||
interface MessageSigningConfig { | ||
String getSigningMessage(); | ||
String getTargetHeaderName(); | ||
String generateTargetHeaderValue(String messageSignature, String approovTokenHeader); | ||
} | ||
|
||
interface MessageSigningConfigFactory { | ||
MessageSigningConfig generateMessageSigningConfig(Request request, String approovTokenHeader); | ||
} | ||
|
||
/* message signing configuration | ||
* This class is used to configure the message signing feature. The message signature can be computed based on the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment needs to be updated to reflect the changes to the class. It should also explain what the message signing behaviour is, which bits are included in the message, the order and any transformation that is performed (b64 encode the body). |
||
* request URL, the headers specified in signedHeaders in the order in which they are listed and, optionally, the | ||
* body of the message. The signature will be added to the request headers using the header name specified in header. | ||
* You can have multiple configurations for different domains and a default '*' configuration that will be used if no | ||
* specific configuration is found for a domain. | ||
*/ | ||
public class DefaultMessageSigningConfigFactory implements MessageSigningConfigFactory { | ||
// the name of the header that will be used to send the message signature | ||
protected String targetHeader; | ||
// the list of headers to include in the message to be signed, in the order they should be added | ||
protected ArrayList<String> signedHeaders; | ||
|
||
// constructor | ||
public ApproovMessageSigningConfig(String targetHeader) { | ||
if (targetHeader == null || targetHeader.isEmpty()) | ||
throw new IllegalArgumentException("The target header must be specified"); | ||
this.targetHeader = targetHeader; | ||
this.signedHeaders = new ArrayList<>(); | ||
} | ||
|
||
/* Get/set methods */ | ||
|
||
/* Get target header */ | ||
public String getTargetHeader() { | ||
return targetHeader; | ||
} | ||
|
||
/* Add a header to the list of signed headers: NOTE the sequence of headers DOES matter */ | ||
public DefaultMessageSigningConfigFactory addSignedHeader(String header) { | ||
if (header == null || header.isEmpty()) | ||
throw new IllegalArgumentException("The header must be specified"); | ||
signedHeaders.add(header); | ||
return this; | ||
} | ||
|
||
MessageSigningConfig generateMessageSigningConfig(Request request, String approovTokenHeader){ | ||
StringBuilder usedHeadersSpec = new StringBuilder(); | ||
StringBuilder message = new StringBuilder(); | ||
// 1. Add the Method to the message | ||
message.append(request.method()); | ||
message.append("\n"); | ||
// 2. add the URL to the message, followed by a newline | ||
message.append(request.url()); // TODO make sure this includes all the URL params if there are any | ||
message.append("\n"); | ||
// 3. add the Approov token header to the message | ||
List<String> values = request.headers(approovTokenHeader); // make sure the okhtp lookup works whatever the case used on the header name | ||
usedHeadersSpec.append(approovTokenHeader.toLowerCase() + "," + values.size()) | ||
for (String value : values) { | ||
message.append(approovTokenHeader.toLowerCase()).append(":"); | ||
if (value != null) { | ||
message.append(value); | ||
} | ||
message.append("\n"); | ||
} | ||
|
||
// 4. add the required headers to the message as 'headername:headervalue', where the headername is in | ||
// lowercase | ||
if (messageSigningConfig.getSignedHeaders() != null) { | ||
for (String header : messageSigningConfig.signedHeaders) { | ||
// add one headername:headervalue\n entry for each header value to be included in the signature | ||
List<String> values = request.headers(header); | ||
if (values != null && values.length() > 0) { | ||
usedHeadersSpec.append(approovTokenHeader.toLowerCase()+ "," + values.size()) | ||
for (String value : values) { | ||
message.append(header.toLowerCase()).append(":"); | ||
if (value != null) { | ||
message.append(value); | ||
} | ||
message.append("\n"); | ||
} | ||
} | ||
} | ||
} | ||
String finalUsedHeadersSpec = usedHeadersSpec.String() | ||
message.append(finalUsedHeadersSpec) | ||
|
||
// add the body to the message | ||
okhttp3.RequestBody body = request.body(); | ||
if (body != null) { | ||
Buffer buffer = new Buffer(); | ||
body.writeTo(buffer); | ||
message.append(buffer.readUtf8()); | ||
} | ||
return new DefaultMessageSigningConfig( | ||
); | ||
} | ||
} | ||
|
||
public class DefaultMessageSigningConfig implements MessageSigningConfig | ||
// the name of the header that will be used to send the message signature | ||
private String targetHeader; | ||
// the list of headers with counts that are expected by the server and were also included in the message to be signed | ||
private String usedHeadersSpec; | ||
// the message to be signed | ||
private String message; | ||
|
||
DefaultMessageSigningConfig(String targetHeader, String usedHeadersSpec, String message) { | ||
this.targeHeader = targetHeader; | ||
this.usedHeaderSpec = usedHeaderSpec; | ||
this.message = message; | ||
} | ||
|
||
public String getTargetHeaderName(){ return targetHeader } | ||
public String getSigningMessage() { return usedHeadersSpec } | ||
public String generateTargetHeaderValue(String messageSignature) { | ||
return usedHeaderSpec + ";" + messageSignature; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
import okhttp3.OkHttpClient; | ||
import okhttp3.Request; | ||
import okhttp3.Response; | ||
import okio.Buffer; | ||
|
||
// ApproovService provides a mediation layer to the Approov SDK itself | ||
public class ApproovService { | ||
|
@@ -82,6 +83,9 @@ public class ApproovService { | |
// set of URL regexs that should be excluded from any Approov protection, mapped to the compiled Pattern | ||
private static Map<String, Pattern> exclusionURLRegexs = null; | ||
|
||
// message signing configurations mapped to a domain | ||
private static Map<String,ApproovMessageSigningConfig> messageSigningConfigs = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Richard convinced me to remove the map - just have a single config. (@richardmtaylor : I'm now a bit concerned about the different headers that will be used for different endpoints / apis - if we include all of these header names in the message signing target header for all requests are we not potentially leaking information about other APIs that are in use in the app to 3rd parties that shouldn't have this information? - having a config per domain would hopefully avoid this.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good point, but I'm not sure it is worth the complexity to hide this. We will only send it on domains that have Approov tokens added anyway. We shouldn't be signing domains that are not Approov protected, since we rely on an Approov token to prevent replay attacks anyway. And all we are leaking is the names of headers, so don't think it is that big of a deal. |
||
|
||
/** | ||
* Construction is disallowed as this is a static only class. | ||
*/ | ||
|
@@ -118,6 +122,31 @@ public static void initialize(Context context, String config) { | |
} | ||
} | ||
|
||
/** | ||
* Sets the message signing configuration. If this is set, then a message signature will be computed based on the | ||
* request URL, the headers specified in signedHeaders in the order in which they are listed and, optionally, the | ||
* body of the message. The signature will be added to the request headers using the header name specified in header. | ||
* | ||
* To unset message signing, call this function as setMessageSigning(nil, nil, false) | ||
* | ||
* @param messageConfig is the ApproovMessageSingingConfig object that contains the message signing configuration | ||
* @param domain is the domain for which the message signing configuration should be used; use '*' for the default configuration which is applied when no specific configuration is found for a domain | ||
*/ | ||
public static synchronized void setMessageSigning(ApproovMessageSigningConfig messageConfig, String domain) throws ApproovException { | ||
if ((messageConfig == null) || (domain == null) || (domain.isEmpty())) { | ||
throw new ApproovException("The message signing configuration and domain must be specified"); | ||
} else { | ||
// We check if this is the first message configuration that is created | ||
if (messageSigningConfigs == null) { | ||
messageSigningConfigs = new HashMap<>(); | ||
} | ||
// Add the configuration and domain to the map | ||
messageSigningConfigs.put(domain, messageConfig); | ||
Log.d(TAG, "setMessageSigning for domain: " + domain); | ||
} | ||
|
||
} | ||
|
||
/** | ||
* Sets a flag indicating if the network interceptor should proceed anyway if it is | ||
* not possible to obtain an Approov token due to a networking failure. If this is set | ||
|
@@ -166,8 +195,12 @@ public static synchronized void setDevKey(String devKey) throws ApproovException | |
* @param header is the header to place the Approov token on | ||
* @param prefix is any prefix String for the Approov token header | ||
*/ | ||
public static synchronized void setApproovHeader(String header, String prefix) { | ||
public static synchronized void setApproovHeader(String header, String prefix) throws ApproovException { | ||
Log.d(TAG, "setApproovHeader " + header + ", " + prefix); | ||
// We only allow this to proceed if message signing is not enabled since changing the header after configuring the message signing is not allowed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to throw here anymore. The active approov header name is passed to the message signing config every time it processes a request. |
||
if (messageSigningConfigs != null) { | ||
throw new ApproovException("The Approov Token header cannot be changed after configuring the message signing"); | ||
} | ||
approovTokenHeader = header; | ||
approovTokenPrefix = prefix; | ||
okHttpClient = null; | ||
|
@@ -396,7 +429,7 @@ public static void setDataHashInToken(String data) throws ApproovException { | |
* is not possible to use the networking interception to add the token. This will | ||
* likely require network access so may take some time to complete. If the attestation fails | ||
* for any reason then an ApproovException is thrown. This will be ApproovNetworkException for | ||
* networking issues wher a user initiated retry of the operation should be allowed. Note that | ||
* networking issues where a user initiated retry of the operation should be allowed. Note that | ||
* the returned token should NEVER be cached by your app, you should call this function when | ||
* it is needed. | ||
* | ||
|
@@ -432,6 +465,7 @@ else if (approovResults.getStatus() != Approov.TokenFetchStatus.SUCCESS) | |
return approovResults.getToken(); | ||
} | ||
|
||
|
||
/** | ||
* Gets the signature for the given message. This uses an account specific message signing key that is | ||
* transmitted to the SDK after a successful fetch if the facility is enabled for the account. Note | ||
|
@@ -629,7 +663,7 @@ public static synchronized OkHttpClient getOkHttpClient() { | |
Log.d(TAG, "Building new Approov OkHttpClient"); | ||
ApproovTokenInterceptor interceptor = new ApproovTokenInterceptor(approovTokenHeader, | ||
approovTokenPrefix, bindingHeader, proceedOnNetworkFail, substitutionHeaders, | ||
substitutionQueryParams, exclusionURLRegexs); | ||
substitutionQueryParams, exclusionURLRegexs, messageSigningConfigs); | ||
okHttpClient = okHttpBuilder.certificatePinner(pinBuilder.build()).addInterceptor(interceptor).build(); | ||
} else { | ||
// if the ApproovService was not initialized then we can't add Approov capabilities | ||
|
@@ -688,6 +722,9 @@ class ApproovTokenInterceptor implements Interceptor { | |
// set of URL regexs that should be excluded from any Approov protection, mapped to the compiled Pattern | ||
private Map<String, Pattern> exclusionURLRegexs; | ||
|
||
// message signing configuration, if any | ||
private Map<String,ApproovMessageSigningConfig> messageSigningConfigs; | ||
|
||
/** | ||
* Constructs a new interceptor that adds Approov tokens and substitute headers or query | ||
* parameters. | ||
|
@@ -702,7 +739,8 @@ class ApproovTokenInterceptor implements Interceptor { | |
*/ | ||
public ApproovTokenInterceptor(String approovTokenHeader, String approovTokenPrefix, String bindingHeader, | ||
boolean proceedOnNetworkFail, Map<String, String> substitutionHeaders, | ||
Set<String> substitutionQueryParams, Map<String, Pattern> exclusionURLRegexs) { | ||
Set<String> substitutionQueryParams, Map<String, Pattern> exclusionURLRegexs, | ||
Map<String,ApproovMessageSigningConfig> messageSigningConfigs) { | ||
this.approovTokenHeader = approovTokenHeader; | ||
this.approovTokenPrefix = approovTokenPrefix; | ||
this.bindingHeader = bindingHeader; | ||
|
@@ -719,6 +757,7 @@ public ApproovTokenInterceptor(String approovTokenHeader, String approovTokenPre | |
} | ||
} | ||
this.exclusionURLRegexs = new HashMap<>(exclusionURLRegexs); | ||
this.messageSigningConfigs = messageSigningConfigs; | ||
} | ||
|
||
@Override | ||
|
@@ -738,6 +777,8 @@ public Response intercept(Chain chain) throws IOException { | |
|
||
// request an Approov token for the domain | ||
String host = request.url().host(); | ||
|
||
// fetch the Approov token for the domain | ||
Approov.TokenFetchResult approovResults = Approov.fetchApproovTokenAndWait(host); | ||
|
||
// provide information about the obtained token or error (note "approov token -check" can | ||
|
@@ -766,16 +807,16 @@ public Response intercept(Chain chain) throws IOException { | |
// we successfully obtained a token so add it to the header for the request | ||
request = request.newBuilder().header(approovTokenHeader, approovTokenPrefix + approovResults.getToken()).build(); | ||
else if ((approovResults.getStatus() == Approov.TokenFetchStatus.NO_NETWORK) || | ||
(approovResults.getStatus() == Approov.TokenFetchStatus.POOR_NETWORK) || | ||
(approovResults.getStatus() == Approov.TokenFetchStatus.MITM_DETECTED)) { | ||
(approovResults.getStatus() == Approov.TokenFetchStatus.POOR_NETWORK) || | ||
(approovResults.getStatus() == Approov.TokenFetchStatus.MITM_DETECTED)) { | ||
// we are unable to get an Approov token due to network conditions so the request can | ||
// be retried by the user later - unless this is overridden | ||
if (!proceedOnNetworkFail) | ||
throw new ApproovNetworkException("Approov token fetch for " + host + ": " + approovResults.getStatus().toString()); | ||
} | ||
else if ((approovResults.getStatus() != Approov.TokenFetchStatus.NO_APPROOV_SERVICE) && | ||
(approovResults.getStatus() != Approov.TokenFetchStatus.UNKNOWN_URL) && | ||
(approovResults.getStatus() != Approov.TokenFetchStatus.UNPROTECTED_URL)) | ||
(approovResults.getStatus() != Approov.TokenFetchStatus.UNKNOWN_URL) && | ||
(approovResults.getStatus() != Approov.TokenFetchStatus.UNPROTECTED_URL)) | ||
// we have failed to get an Approov token with a more serious permanent error | ||
throw new ApproovException("Approov token fetch for " + host + ": " + approovResults.getStatus().toString()); | ||
|
||
|
@@ -862,6 +903,69 @@ else if (approovResults.getStatus() != Approov.TokenFetchStatus.UNKNOWN_KEY) | |
} | ||
} | ||
|
||
// if message signing is enabled, add the signature header to the request | ||
if (messageSigningConfigs != null) { | ||
// Match the domain of the request URL against the configured message signing domains | ||
String domain = request.url().host(); | ||
ApproovMessageSigningConfig messageSigningConfig = messageSigningConfigs.get(domain); | ||
if (messageSigningConfig == null) { | ||
// If no specific configuration is found for the domain, use the default configuration | ||
domain = "*"; | ||
messageSigningConfig = messageSigningConfigs.get(domain); | ||
} | ||
if (messageSigningConfig == null) { | ||
// If no default configuration is found, do not sign the message | ||
return chain.proceed(request); | ||
} | ||
Log.d(TAG, "Signing message with configuration for domain: " + domain + ": " + (messageSigningConfig.getSignApproovToken() ? ", " + approovTokenHeader + " **** " : "") + " headers " + messageSigningConfig.getSignedHeaders() + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code should all move into the "public String generateMessage(request Request, String approovTokenHeader);" in the DefaultMessageSigningConfig implementation. |
||
(messageSigningConfig.getSignBody() ? ", body" : "")); | ||
// build the message to sign, consisting of the URL, the names and values of the included headers and | ||
// the body, if enabled, where each entry is separated from the next by a newline character | ||
StringBuilder message = new StringBuilder(); | ||
// 1. add the URL to the message, followed by a newline | ||
message.append(request.url()); | ||
message.append("\n"); | ||
// 2. Add the Method to the message | ||
message.append(request.method()); | ||
message.append("\n"); | ||
// 3. add the Approov token to the message, if required (TODO: Note we add the approov token and the prefix!?????) | ||
if (messageSigningConfig.signApproovToken) { | ||
if (approovResults.getToken() != null) { | ||
message.append(approovTokenPrefix + approovResults.getToken()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should read the approov header from the request instead of recreating the contents here - it is just a safer approach as the same process will occur on the server side -once you migrate the code, into the DefaultRequestSigningConfig you will have to do that anyway. |
||
} | ||
message.append("\n"); | ||
} | ||
// 4. add the required headers to the message as 'headername:headervalue', where the headername is in | ||
// lowercase | ||
if (messageSigningConfig.getSignedHeaders() != null) { | ||
for (String header : messageSigningConfig.signedHeaders) { | ||
// add one headername:headervalue\n entry for each header value to be included in the signature | ||
List<String> values = request.headers(header); | ||
for (String value : values) { | ||
message.append(header.toLowerCase()).append(":"); | ||
if (value != null) { | ||
message.append(value); | ||
} | ||
message.append("\n"); | ||
} | ||
} | ||
} | ||
// add the body to the message | ||
if (messageSigningConfig.getSignBody()) { | ||
okhttp3.RequestBody body = request.body(); | ||
if (body != null) { | ||
Buffer buffer = new Buffer(); | ||
body.writeTo(buffer); | ||
message.append(buffer.readUtf8()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to b64 encode the body to make sure there are no control characters in the string when it is passed to the approov SDK signing method. |
||
} | ||
} | ||
// compute the signature and add it to the request (passing on any exceptions that may occur) | ||
if (messageSigningConfig.targetHeader != null && !messageSigningConfig.targetHeader.isEmpty()) { | ||
String signature = ApproovService.getMessageSignature(message.toString()); | ||
request = request.newBuilder().header(messageSigningConfig.targetHeader, signature).build(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once we have the signature, call the 'generateHeaderValue' method on the MessageSigningConfig to generate the string value for the header instead of just adding the signature directly. |
||
} | ||
} | ||
|
||
// proceed with the rest of the chain | ||
return chain.proceed(request); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a bunch of changes here to indicate a new approach. The idea is to construct a message and a signature that can be used to correctly generate and verify the signature on the server side. To do this, while building the headers that will be added to the message we record the names of the headers and the counts of the values as a string. The generated string is then added to the message that will be signed.
The intention then is that this value is added to the header used to hold the signature along with the signature itself. The server side can then break apart the value and use it to reconstruct the message that should be used to verify the signature.
Unfortunately this implementation has a flaw, as headers not in a message but that may influence the behaviour of an endpoint can be freely added to the request and no detection would be made. Ultimately, I think the only legitimate defense against this is that the server side signature chieck also needs to know the set of headers that are expected to be included in the signature and their order. In essence, I don't think it is really possible to have the client side safely communicate to the server the set of headers that it should check in order to evaluate the legitimacy of a signature.
In summary, Ivo and Richard, I think we need some more discussions before we can fix on a nimplementation.