Feature/message signing #6
Conversation
This reverts commit 1c9b3e2.
…ble changing header name if message signing has been enabled
…ap to configurations
There was a problem hiding this comment.
@ivolz I think my comments indicate the direction we should go but it is worth checking with @richardmtaylor tomorrow to make sure we are all on the same page.
| import java.util.ArrayList; | ||
|
|
||
| /* 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.
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).
| * 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 ApproovMessageSigningConfig { |
There was a problem hiding this comment.
We should add a new interface as follows:
interface MessageSigningConfig {
public String getTargetHeader();
public String generateSigningMessage(Request request, String approovTokenHeader);
public String generateTargetHeaderValue(String messageSignature, String approovTokenHeader);
}
then, this class should instead be called DefaultMessageSigningConfig and it should implement MessageSigningConfig.
| return targetHeader; | ||
| } | ||
| /* Get signed headers */ | ||
| public ArrayList<String> getSignedHeaders() { |
There was a problem hiding this comment.
I don't think we need the getter, I can't think of any reason that would be necessary.
There was a problem hiding this comment.
Debugging purposes perhaps only?
| return signedHeaders; | ||
| } | ||
| /* Add a header to the list of signed headers: NOTE the sequence of headers DOES matter */ | ||
| public void addSignedHeader(String header) { |
There was a problem hiding this comment.
It makes sense to make addSignedHeader chainable to simplify the configuration code. So it would be:
public DefaultMessageSigningConfig addSignedHeader(String header); (returning 'this' at the end of the method)
It would make configuration work like this:
aConfig = new DefaultMessageSigningConfig("header").
addSignedHeader("header1").
addSignedHeader("header2");
| // the list of headers to include in the message to be signed, in the order they should be added | ||
| protected ArrayList<String> signedHeaders; | ||
| // true if the message body should also be signed | ||
| protected Boolean signBody; |
There was a problem hiding this comment.
After chatting with Richard - remove the following: signBody, signApproovToken, signURL, and signMethod
| // 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.
This code should all move into the "public String generateMessage(request Request, String approovTokenHeader);" in the DefaultMessageSigningConfig implementation.
| // 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.
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.
| if (body != null) { | ||
| Buffer buffer = new Buffer(); | ||
| body.writeTo(buffer); | ||
| message.append(buffer.readUtf8()); |
There was a problem hiding this comment.
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.
| @@ -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.
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.
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.
| 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.
No need to throw here anymore. The active approov header name is passed to the message signing config every time it processes a request.
| // 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.
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.
Proposed changes that will need to be discussed before being adopted in a final form.
| import java.util.ArrayList; | ||
| import okhttp3.Request; | ||
|
|
||
| /* Add the following interfaces */ |
There was a problem hiding this comment.
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.
No description provided.