Switch JWT library implementations

[peternied]

After reviewing the different libraries for constructing JWT authentication tokens, for the OnBehalfOf feature should use Nimbus JOSE + JWT

Exit Criteria

• Add a checkstyle rule to detect including references to org.apache.cxf.rs.security.jose
• Remove all references to org.apache.cxf.rs.security.jose [15] files impacted
• Replace all unfulfilled references with com.nimbusds.jose.* components
• Add backwards combability tests for any features that have previously shipped

Additional Context

• Prototype main...peternied:security:nimbus-jose-jwt

[stephen-crawford]

[Triage] This issue seems to have clearly outlined closure criteria and is the result of guidance around how to improve our overall security posture. We will need this to be backwards compatible so that the a token made with the CXF implementation can then go into the Nimbus version.

Given the prototype Peter made, this seems like something we can mark good first issue.

[MaciejMierzwa]

Started looking into it

[peternied]

Thanks @MaciejMierzwa for looking into this, I've marked this issue assigned to you. Please comment on this issue as you make progress or alternatively discussion can occur in a pull request.

[MaciejMierzwa]

Hi, here's the branch and draft PR I'm pushing my changes to: #3421
So far so good although there are more differences between library implementations then I hoped for. Also, I noticed that nimbus library doesn't accept shorter keys for signing JWT, seems like cxf was adding padding if secret key was too short, nimbus on the other hand forces user to use proper length secret (512 bits for HMAC512 etc)

[MaciejMierzwa]

Hi, for visibility I'll write here instead of PR.
There are few differences between how both libraries dealt with implementing specification which caused issues.

1. For HMAC keys it's suggested that the key is at least as long as defined by the hash function. Shorter keys make it significantly to break hashing. The specification says that shorter keys should be padded: https://www.rfc-editor.org/rfc/rfc2104#section-2
   CXF was apparently padding the keys, nibus on the other hand throws an error if key is too short. I added util method that pads key with "\0" which seems to do the trick. One thing I'm thinking about is introducing a utility class and moving the method there. https://github.com/MaciejMierzwa/security/blob/7e2c6ca4b9a4fa755e4ca9c81ff156e7f2eab3f1/src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L108C26-L108C35
2. There is similar difference in implementation for audience claims. Specification suggests that audience claims is array of strings, in special cases it can be single string. Nimbus is parsing audience claims as array, no way around it: relevant part of library code. There are a lot of changes in tests because of that.
3. I removed cxf-rt-rs-security-jose package from dependencies. There are 3 other cxf packages that would be ok to remove. Was intention to remove all of those?

[MaciejMierzwa]

@cwperks I'd like to ask for input from your side?
I've run couple ci runs doing minor changes to code and I was getting different types of errors all the time.
Seems like backward-compatibility tests are pretty flaky.

1. Here's one that mentions build errors. Is the code compiled here different then the one compiled for unit testing?
   https://github.com/opensearch-project/security/actions/runs/6420709692/job/17433407075?pr=3421
2. A couple commits before it actually passed: https://github.com/opensearch-project/security/actions/runs/6419670321/job/17430002039
3. Here is run that caused errors during serialization/deserialization communication between nodes., while no major changes in the code were made (style, changes in tests) https://github.com/opensearch-project/security/actions/runs/6407647266/job/17394880689
   Overall seems like build artifacts from recent changes in the opensearch/os-security custom serialization might be causing it.

EDIT. it was probably flakiness, seems to work better now

[peternied]

@MaciejMierzwa I've checked off a number of items from the issues description - there is only 1 outstanding item which is making sure that previous configurations are compatible with this change. I believe the changes you made around how padding is handled should verify this - but I'd ask that we add a new tests that generates a token with CXF - save it into a string, then read it with the new system to ensure backwards compability - that should close out this issue,

Feel free to do that as part of your current PR [1] or open a new one afterward.

• [1] Switch JWT library implementations from cxf to nimbus #3421

[MaciejMierzwa]

Hi @peternied, please take a look at this: https://github.com/MaciejMierzwa/security/tree/nimbus-jose-jwt-cxf-test
In commit: dd02d0c I added proof of concept tests that are dealing with cxf/nimbus compability, how padding affects generated jwts etc. Since cxf library is supposed to be gone I guess there is no need to merge it to the main repo so I created separate branch for it.

Basically I tested 2 scenarios: Create jwt using cxf, encode it, parse it using nimbus or do it other way around. Only challenge is the padding that's added for case of too short secret - nimbus library wouldn't accept it, cxf would.

[peternied]

Basically I tested 2 scenarios: Create jwt using cxf, encode it, parse it using nimbus or do it other way around. Only challenge is the padding that's added for case of too short secret - nimbus library wouldn't accept it, cxf would.

@MaciejMierzwa In my mind, the test is to create a JWT with a key using the old system, then use the new system with the same key can still be able to process read + validate the contents of that JWT - is that possible?

[peternied]

You wouldn't save the generation system, but the encrypted token would be checked in with the key in the test case - so the libraries are unneeded.

[RyanL1997]

Resolve by the merge of #3421