[CXF-8535] Query missing from signature request-target AND also to add digest only if request has body

[AnilKumarHurkadli]

[CXF-8535] Query missing from signature request-target AND also to add digest only if request has body AND Fix to avoid decoding of the path parameters

The following fixes are incorporated in this PR

1. The issue was instead of using getRequestUri of URI info to consider the query parameters in the request-target , getAbsolutePath was used which truncates the query parameters in VerifySignatureFilter and results in Invalid Signature while verifying.

2. In addition to that, digest will be added to the required headers only if the request has payload/body.

Test cases are also been incorporated for the above fix

3. Fix to avoid decoding of the path parameters by using rawpath instead of path otherwise will result in invalid signatures

[davidkarlsen]

@coheigea PTAL

[reta]

@AnilKumarHurkadli I just realized there are no testcases for the case when message body is not present, exactly the flow you are implementing. Could you please add a couple please?

[AnilKumarHurkadli]

@reta Sure will add the test case for which InvalidDataToVerifySignatureException will not be thrown if the body is null and passes fine. The issue was encountered specifically while testing DELETE method which "usually" do not have body and "Digest" header gets added by default in the required headers and is NOT removed as it was not satisfying below condition

if ((requestor && (status < 200 || status >= 300 || status == 204))
|| (!requestor && ("GET".equalsIgnoreCase(method) || "HEAD".equalsIgnoreCase(method)))) {
signedHeaders.remove("digest");
}

And was throwing InvalidDataToVerifySignatureException with message "Not all of the required headers are signed"

[AnilKumarHurkadli]

@reta Have incorporated the test case described in the conversation above

[reta]

👍 thank you, LGTM

[AnilKumarHurkadli]

@reta Path parameters were being decoded while verifying the signatures and hence there were mismatch in the request-target , have provided fix to avoid decoding of the path parameters by using rawpath instead of path

[reta]

@AnilKumarHurkadli thank you, makes sense to me: it is unclear why previously the uri.getPath() was combined with uri.getRawQuery(), either getPath() + getQuery() or getRawPath() + getRawQuery() should have been followed (imho), thank you for fixing.

[AnilKumarHurkadli]

@reta Thank you for reviewing the changes , yes rawpath and rawquery works well as it does not decodes and retains as is for verification.

[reta]

@coheigea from the implementation perspective - LGTM, but from the spec perspective would be great to hear your opinion, thank you.

[coheigea]

The changes look fine to me, but it is causing a test failure in systests/rs-security (JAXRSHTTPSignatureTest) can you take a look?

[AnilKumarHurkadli]

The changes look fine to me, but it is causing a test failure in systests/rs-security (JAXRSHTTPSignatureTest) can you take a look?

Sure @coheigea will take a look why that particular case is failing and update accordingly

[AnilKumarHurkadli]

The changes look fine to me, but it is causing a test failure in systests/rs-security (JAXRSHTTPSignatureTest) can you take a look?
@coheigea , For the below particular test case which is failing , if the digest is not available in the header then body will be returned as null in DigestVerifier as digest header does not exists in the request which is stricter check, can this test case be removed ?

@test
public void testMissingDigestHeader() throws Exception {

URL busFile = JAXRSHTTPSignatureTest.class.getResource("client.xml");

CreateSignatureInterceptor signatureFilter = new CreateSignatureInterceptor();
signatureFilter.setAddDigest(false);
KeyStore keyStore = KeyStore.getInstance("JKS");
keyStore.load(ClassLoaderUtils.getResourceAsStream("keys/alice.jks", this.getClass()),
"password".toCharArray());
PrivateKey privateKey = (PrivateKey)keyStore.getKey("alice", "password".toCharArray());
assertNotNull(privateKey);

List headerList = Arrays.asList("accept", "(request-target)");
MessageSigner messageSigner = new MessageSigner(keyId -> privateKey, "alice-key-id", headerList);
signatureFilter.setMessageSigner(messageSigner);

String address = "http://localhost:" + PORT + "/httpsigprops/bookstore/books";
WebClient client =
WebClient.create(address, Collections.singletonList(signatureFilter), busFile.toString());
client.type("application/xml").accept("application/xml");

Response response = client.post(new Book("CXF", 126L));
assertEquals(400, response.getStatus());
}

OR

to have additional check to add digest in the required headers for POST or PUT method in the below condition in MessageVerifier?

From ,
if (!signedHeaders.contains("digest") && messageBody != null && messageBody.length > 0) {
signedHeaders.add("digest");
}
To add POST or PUT check as below,
if (!signedHeaders.contains("digest") && ((messageBody != null && messageBody.length > 0)
|| ("POST".equalsIgnoreCase(method) || "PUT".equalsIgnoreCase(method)))) {
signedHeaders.add("digest");
}

[coheigea]

I think the test-case is valid, so please instead consider adding POST/PUT as you suggested above.

[AnilKumarHurkadli]

I think the test-case is valid, so please instead consider adding POST/PUT as you suggested above.

@coheigea @reta I have fixed the test case failure that was happening in systests/rs-security (JAXRSHTTPSignatureTest) by adding the required checks as discussed in MessageVerifier

[reta]

Thank you @AnilKumarHurkadli !