Allow impls to continue supporting stronger encryption by default

[dblevins]

I still think mp.jwt.decrypt.key.algorithm should support a list, but I'm happy to table that for MP JWT 3.0 so there's time to discuss -- we're out of runway for that kind of discussion/agreement.

The one tweak we would need before MP JWT 2.1 goes final is to remove the default for mp.jwt.decrypt.key.algorithm. RSA-OAEP has been deprecated for a few years and our MP JWT 2.0 impl supports RSA-OAEP, RSA-OAEP-256, RSA-OAEP-384 and RSA-OAEP-512 out of the box. It also supports ECDH-ES* algorithms.

The addition of mp.jwt.decrypt.key.algorithm is very good, but the default is unsafe and breaks existing applications. They would need to make a code change to re-enable the safer algorithms that worked by default in our MP JWT 2.0 impl.

When we added the first config options in MP JWT 1.1 we stated that when the options were not used, you got the same behavior that you got before the option was added. I've attempted to use the same language for mp.jwt.decrypt.key.algorithm so existing MP JWT 2.0 apps can still run with the safer algorithms without change.

This PR tweaks the TCK as well.

We'd be very happy to vote +1 on a MP JWT 2.1 with this change. As well, provided we can get our own releasing done in time, we may even be able to help with a CCR for the MP JWT 2.1 final release.

[starksm64]

Given that RSA-OAEP is only recommended for backwards compatibility in https://www.rfc-editor.org/rfc/rfc8017#section-7.1.1 due to the use of SHA-1, defaulting to RSA-OAEP does not seem wise. I would agree that allowing the vendor to default to whatever they view as reasonable makes sense.

[rdebusscher]

The section "Required and Recommended MP-JWT Headers and Claims" (interoperability.asciidoc) still says that RSA-OAEP is required as alg value which contradicts the possibility to have RSA-OAEP-256.

[dblevins]

@rdebusscher I've updated the interoperability.asciidoc so it aligns with the description of mp.jwt.decrypt.key.algorithm. Specifically it says the values can be RSA-OAEP or RSA-OAEP-256 and that values such as RSA-OAEP-384, RSA-OAEP-512 and others are optional. I've done the same with the alg description.

As this section is on interoperability, one potential improvement might be to explicitly say "are optional and may not be portable"

Thoughts?

[rdebusscher]

The requirements around RSA-OAEP are contradictory in the spec, see my comment.

[sberyozkin]

It can not be done for MP JWT 2.1 and as I've proposed earlier instead of starting another round of discussion in this PR please start a public discussion on the dev list

Dismissing my change request as I think Rudy's comment captures why it is a problematic change for 2.1

[sberyozkin]

I believe everyone agrees RSA-OAEP-256 should become a default value, but MP JWT 2.0 gives an expectation that an RSA-OAEP encrypted token must be accepted and the users will expect it working - therefore allowing MP JWT 2.1 choose to default to RSA-OAEP-256 is unfortunately a breaking change for 2.1.

I believe now that we've introduced a tool to let users choose RSA-OAEP-256 the problem of having RSA-OAEP by default in 2.1 is not a problem at all.

FYI, I asked the question the other day on the Jose list about the security of RSA-OAEP, and while N. Madden did not approve the use of RSA-OAEP as such, no any known vulnerability of RSA-OAEP exists. Just saying - it is not as insecure as it may seem as SHA1 is only providing one of the inputs to the encryption. And we have a property now to let users switch to an SHA-256 based one.

FYI, the main reason RSA-OAEP was selected in 1.2 as a default algorithm was because it has a Recommended + status which according to JWA spec means:

The use of "+" in the Implementation Requirements column indicates
   that the requirement strength is likely to be increased in a future
   version of the specification.

while RSA-OAEP-256 has an Optional status.

This spec message (and FYI I could not get a clear answer on the Jose list what were the reasons behind it) can be ignored now for the purpose of the MP-JWT spec evolution, but I'd like to explain the reasoning why I chose RSA-OAEP at a time as a default value.

[sberyozkin]

@teddyjtorres FYI

[dblevins]

Just to be clear there is not agreement that RSA-OAEP-256 should become a default value. Specifically, the disagreement is around default rules that require implementations return 401 responses for RSA-OAEP-* algorithms in scenarios where users did not explicitly set the new mp.jwt.decrypt.key.algorithm property. Our implementation supports both RSA-OAEP* and ECDH_ES* and existing MP JWT 2.0 applications would break with this new rule. Understood that apps that used anything other than RSA-OAEP were not portable from an MP JWT 2.0 perspective. It's the new rule that these existing apps now stop functioning by default that we'd need to change in order to implement the MP JWT 2.1 spec.

We are completely fine returning 401 responses when people did explicitly set the new mp.jwt.decrypt.key.algorithm property.

[dblevins]

We're also not comfortable with the plan that apps that did not set mp.jwt.decrypt.key.algorithm will be required to break again in MP JWT 3.0.

[sberyozkin]

@dblevins

Our implementation supports both RSA-OAEP* and ECDH_ES* and existing MP JWT 2.0 applications would break with this new rule.

Which new rule are you referring to ? Introduction of the new property ? It is an optional property - so it does not change anything for MP JWT 2.0 applications. You can continue using whatever other algorithm you prefer. We support all the algorithms possible too, no TCK test exists enforcing that using anything other than the formally supported algorithms must fail.

Understood that apps that used anything other than RSA-OAEP were not portable from an MP JWT 2.0 perspective. It's the new rule that these existing apps now stop functioning by default that we'd need to change in order to implement the MP JWT 2.1 spec.

How is this an MP JWT 2.1 problem, where MP JWT 2.1 is a minor release ? You are talking about non-complaint MP-JWT 2.0 applications - no property existed in 2.0 for specifying the decryption algorithms and RSA-OAEP was set to be a required algorithm, introduction of the new optional property does not change it, so please don't try to offload this problem to the minor spec release. And I'm not sure the problem exists - like I said we also support more algorithms - I certainly do not expect our users be affected by this new property as well :-)

What is definitely the case though is that your proposed PR is a breaking change and hope you appreciate it can't be done for 2.1

[sberyozkin]

@dblevins

We're also not comfortable with the plan that apps that did not set mp.jwt.decrypt.key.algorithm will be required to break again in MP JWT 3.0.

I really don't get it. 3.0 will be a major release, so I'm assuming you are referring to the idea of RSA-OAEP-256 becoming a default algorithm in 3.0 ? The default algorithm should not be a random one chosen by whatever a given implementation prefers but the one specified in the spec to support the interoperability. Well, we'll discuss it further in #288, but the major releases are expected to accommodate the breaking changes.

[sberyozkin]

I propose to have signature related spec texts updated in another PR

[sberyozkin]

I'm not sure what this means, where the algorithm may be specified as either the suggested 2 values but then all other algorithms can also be optionally supported.
And as I said - unfortunately it is a breaking change for 2.1.

[dblevins]

I think it would be good to have a call and discuss as I think we're having a disconnect in text and we're closer to agreeing than might be seen. Ultimately, what we're asking for is that we only be required to reject valid JWE tokens when mp.jwt.decrypt.key.algorithm is explicitly set.

There's an MP Technical call Tuesday Sep 20th at 11pm pacific. Would that work for people? If not how about Thursday Sept 22nd at 8am pacific?

[dblevins]

@dblevins

Our implementation supports both RSA-OAEP* and ECDH_ES* and existing MP JWT 2.0 applications would break with this new rule.

Which new rule are you referring to ? Introduction of the new property ? It is an optional property - so it does not change anything for MP JWT 2.0 applications. You can continue using whatever other algorithm you prefer. We support all the algorithms possible too, no TCK test exists enforcing that using anything other than the formally supported algorithms must fail.

This is the new test that's problematic for us:

• RolesAllowedSignEncryptTest>Arquillian.run:138->callEchoRsaOaep256

If we could get a one-line change so that it uses the mp.jwt.decrypt.key.algorithm=RSA-OAEP that would solve the issue for us.

[sberyozkin]

@dblevins Hi, re the call, we meet with Teddy every 2nd Thursday, MP JWT call is on next Thursday though it is on 15.00 Dublin time.

This is the new test that's problematic for us:
RolesAllowedSignEncryptTest>Arquillian.run:138->callEchoRsaOaep256
If we could get a one-line change so that it uses the mp.jwt.decrypt.key.algorithm=RSA-OAEP that would solve the issue for us.

I think what you are proposing here is to introduce a dynamic detection of the algorithm, i.e, the user has not configured any decryption algorithm, but the algorithm is always in the token headers anyway, so lets use it.

This is technically OK, but in general it is discouraged - there must be an expectation on the server side as to which algorithm is expected, since otherwise it is the client who tells the server how to decrypt with the server having no influence on this decision and there was a well known exploit though it was involving a symmetric key signature as far as I recall. It is also important to realize that in production, a given issuer will never be switching the algorithms - it is just not how it works in production - the only thing they may change dynamically is the keys.

So while the dynamic algorithm detection is technically a flexible approach, all it does in fact it introduces a security concern.

We'll talk though more about it in 3.0 but as I said to Rudy earlier - it is part of the bigger multi-tenancy issue IMHO (different setting per different issuers)

Note mp.jwt.public.key.algorithm also has a default value, RS256, so the new property for the decryption algorithm follows the same pattern.

I think the best way forward is indeed modify this TCK test as per your proposal, perhaps blocking the dynamic detection should not be enforced.

Can you please open a new PR to modify RolesAllowedSignEncryptTest as you have proposed and we can keep this one to revisit in 3.0, does it work ?
thanks

[dblevins]

Thanks for the understanding @sberyozkin

You're largely correct on us supporting multiple algorithms. I wouldn't call it a proposal as we're not asking for others to do the same, just wanting to make sure the spec continues to not rule out that approach. It was flexible in that regard in MP JWT 1.0 and 1.1.

I did see mp.jwt.public.key.algorithm, but the description around it describes it as primarily a hint to help key parsing and its use at verification time is a recommendation and there are no tests for it. That said, I'd be fine if we did firm the requirements for it up so that using it as a white list isn't a recommendation and there are tests for it -- just as long as we're clear when not used there is no standard behavior or default. Basically, the same perspective I have for mp.jwt.decrypt.key.algorithm

On submitting a PR, I'm happy to take the win and do that, leaving the spec untouched. However, I do admit it feels odd for the TCK to have a lax perspective and be flexible on this, but not have that flexibility reflected in the spec. Is there anything we'd want to say in the spec to reflect the understanding implementations need only enforce mp.jwt.decrypt.key.algorithm when used?

Last note, can you work with @jclingan to get the MP-JWT calls into the public calendar?

[dblevins]

@sberyozkin thank you again for the understanding, Sergey. We really appreciate it.

[sberyozkin]

Hey @dblevins Np at all, and glad you are back in the MP-JWT space :-), thanks for your input/patience.

Indeed, lets just fix this TCK test issue first and avoid blocking the implementations which prefer to go the dynamic algorithm detection route. Perhaps we can formalize it in 3.0, at least as an optional approach...

Is there anything we'd want to say in the spec to reflect the understanding implementations need only enforce mp.jwt.decrypt.key.algorithm when used?

Not sure anything else is needed to add to the current text - perhaps it can make sense to stress that is is an optional property which would imply it is only acted upon if it is specified.

cheers

[sberyozkin]

@dblevins Let me take care of updating TCK, I'd like to move that negative test to a dedicated test

[dblevins]

@sberyozkin thanks so much! Saw and approved that PR.

What's the call information on the MP JWT call tomorrow?

[dblevins]

@sberyozkin I joined this link I found in the wiki https://redhat.bluejeans.com/2363391609, maybe 5 after. Not sure if that was the right link or if I was a little too late.

[dblevins]

Not sure anything else is needed to add to the current text - perhaps it can make sense to stress that is is an optional property which would imply it is only acted upon if it is specified.

Stressing it's only acted upon when used would be good. One way to achieve that could be to simply delete the line that says there's a default as to me that's what implies it's active even when not specified. I might be interpreting that differently than is intended, so maybe the right question to ask is what behavior should users expect when we say "the default is x"?

[sberyozkin]

@dblevins John should put it back on the calendar

what behavior should users expect when we say "the default is x"?

I've released RC4 (please try) and IMHO we do not need to do anything else before Final.
Right now there is no formal support for the dynamic algorithm detection, and TCK has just been relaxed not to enforce it that the server expectation is set, for the implementations which don't mind where the algorithm is set, to work as they need to.

"the default is x" means that the spec expects the implementations to assert that the server expects x by default. Once we remove it then we immediately move to the dynamic algorithm detection, and as I said earlier, it can be considered as an anti-pattern in some cases and I'd like to avoid getting into it in 2.1.

By removing it, we will require, in 2.1, the implementations to start dynamically support RSA-OAEP/RSA-OAEP-256 - it will have cost side-effects as the implementations will have to parse the tokens themselves to detect the algorithm, before delegating to the underlying Jose library, for no obvious practical reasons - as I said - no any real provider will be switching the algorithms dynamically - don't hesitate to prove me wrong.

[sberyozkin]

@dblevins So, in 2.1, if someone sends an RSA-OAEP-256 token to your implementation then it will work with you having to do anything to enforce any defaults - because TCK does not enforce it - but you should really use the new property to assert that RSA-OAEP-256 is expected because it is a good security pattern.
Then the same token goes to smallrye-jwt 2.1 impl and it will fail - but we'll tell our users - you should set the RSA-OAEP-256 because it is important for the server to set the expectation.
I.e the users will have a choice, and as we've discussed above, it is the choice of your implementation not to require it - TCK has been relaxed to allow for it.

[sberyozkin]

I'd like to re-iterate/repeat - the dynamic algorithm detection is technically cool, but there are no obvious practical reasons and it will add to the cost of processing the tokens.
Therefore at the 2.0 to 2.1 transition moment, if you'd like to assure your users your implementation supports the portability of dealing with such tokens, then you should not recommend them their RSA-OAEP-256 tokens will be accepted without them configuring anything, and advise them to use the new property when dealing with RSA-OAEP-256 and warn, if you'd like, that other implementations may not accept such tokens without the server expectation set.

[sberyozkin]

Hey @dblevins

I've been thinking more about it and I don't see how we can relax it for 2.1, I'm probably repeating myself, but here are the 2 main reasons why:

• Dropping the notion of a server expectation where the server expects a concrete algorithm in 2.1 is a problem, it is an important security mechanism and I'm concerned of just dropping it
• I'd like to highlight it again - there are no production ready applications I'm aware of where the algorithms are switched dynamically or multiple algorithms are used at the same time to create a token for a specific service. So I think we have to be careful with formalizing the dynamic detection - it really should not be about lets do it because it is possible to detect algorithms dynamically - it really should be backed up by the actual production requirements IMHO.

[dblevins]

@sberyozkin Hey Sergey. Apologies for the silence -- had to travel for a family emergency.

Agree with not adding any formalization or requirements for "auto-detecting" algorithms.

My comment was a little different. Do we want to make it clear to users we're not requiring implementations to reject tokens unless mp.jwt.decrypt.key.algorithm is set?

[dblevins]

no any real provider will be switching the algorithms dynamically - don't hesitate to prove me wrong.

I don't know if this is helpful, but the library we use is jose4j. We set up the AlgorithmConstraints as a PERMIT list allowing RSA_OAEP, RSA_OAEP_256, ECDH_ES, ECDH_ES_A128KW, ECDH_ES_A192KW, ECDH_ES_A256KW by default. It's written by Brian Campbell who is a Distinguished Engineer at Ping Identity.

[dblevins]

I've updated the PR so it simply states the default for mp.jwt.decrypt.key.algorithm is a recommendation. This would

• address our concern as then there is room for our defaults to be different
• clarifies to users there is no portable default
• does not prevent other vendors from having RSA-OAEP as a default

[rdebusscher]

I don't agree with the fact that the default (when no value is specified for mp.jwt.decrypt.key.algorithm config value) is an implementation/runtime detail.
This means we loose the portability between runtimes.

[rdebusscher]

Since version 2.1 is released last week, I think we should concentrate on having the list option for mp.jwt.decrypt.key.algorithm parameter for version 3.0 and can drop this PR.

[dblevins]

@rdebusscher 2.1 has not yet gone up for a release vote, so any tags cut can be redone. In terms of agreement on a default, I guess that's the main issue -- we all do not agree. We should only put text like that in the spec in situations where there is agreement between all the implementations and users can reliably count on that behavior. The reality is our implementations have different defaults. We should make sure that users are aware of it.

[dblevins]

This means we loose the portability between runtimes.

And to be clear on this one, there was never portability here to lose. It's clear through this discussion how each of us have handled our out-of-the-box / default behavior has never been the same. Some of us have only allowed what is required by the spec, some of us have allowed more. There's no way to force a consistent behavior without someone's user base being negatively affected by the change. Therefore we should be clear in the spec that we are not testing or enforcing a default and servers may behave differently when mp.jwt.decrypt.key.algorithm is not used.

[rdebusscher]

We should only put text like that in the spec in situations where there is agreement between all the implementations and users can reliably count on that behavior

I have another idea of specifications. Implementations should not decide what and how something goes into a spec, but specs (as a neutral body) must enforce features and behaviours for specifications that are willing to certify against it. Spec texts must be clear and unambiguous so that each implementation is identical (although additional features are possible on top of the spec in an implementation)

And it is perfectly fine that an implementation doesn't implement the entire spec but then they agree not to call them compatible. And for the user it is clear that if it isn't compatible, they can expect differences.

[rdebusscher]

2.1 has not yet gone up for a release vote, so any tags cut can be redone.

Not if we want to have it included in MP 6.0 as OpenLiberty started to implement/integrate it so that ballot can happen. (deadline was last week)

No agreement seems possible and fundamental interpretation differences about specifications between groups. I drop my remarks so other people can continue.

[sberyozkin]

Hey @dblevins Are you OK with getting this one closed now ?

[sberyozkin]

David, if I understand it correctly we can close this one now, thanks