Skip to content
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

Update getSupportedSSLParameters() #215

Closed
wants to merge 1 commit into from

Conversation

sstefonic
Copy link
Contributor

SunJSSE expects that calling getSupportedSSLParameters() will return an SSLParameters object with the full list of supported cipher suites and protocols. Update engineGetSupportedSSLParameters() to match that.

@sstefonic sstefonic assigned sstefonic and cconlon and unassigned sstefonic Jul 23, 2024

/* set cipher suites and protocols to all supported cipher suites and
protocols */
supportedParams.setCipherSuites(WolfSSLUtil.sanitizeSuites(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this cipher suite and protocol list already set when the WolfSSLContext is created, inside createCtx()? Around these lines:

161         /* Auto-populate enabled ciphersuites with supported ones. If suites
162          * have been restricted with wolfjsse.enabledCipherSuites system
163          * security property, the suite list will be filtered in
164          * WolfSSLEngineHelper.sanitizeSuites() to adhere to any
165          * set restrictions */
166         if (WolfSSLUtil.isSecurityPropertyStringSet(
167             "wolfjsse.enabledCipherSuites")) {
168             /* User is overriding cipher suites, set CTX list */
169             this.setCtxCiphers(WolfSSLUtil.sanitizeSuites(ciphersIana));
170         }
171         params.setCipherSuites(WolfSSLUtil.sanitizeSuites(ciphersIana));
172
173         /* Auto-populate enabled protocols with supported ones. Protocols
174          * which have been disabled via system property get filtered in
175          * WolfSSLEngineHelper.sanitizeProtocols() */
176         params.setProtocols(WolfSSLUtil.sanitizeProtocols(
177             this.getProtocolsMask(ctxAttr.noOptions)));

I would think since that is set into the object-level "params", that would also come through here when we call decoupleParams(), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like at line 76 ciphersIana is set to WolfSSL.getCiphersAvailableIana(), returning the list of "enabled ciphers":

        /* Get available wolfSSL cipher suites in IANA format */
        ciphersIana = WolfSSL.getCiphersAvailableIana(this.currentVersion);

Then, at line 81, ciphersIana is passed into WolfSSLCustomUser and is set as the list variable:

        WolfSSLCustomUser ctxAttr = WolfSSLCustomUser.GetCtxAttributes
                          (this.currentVersion, ciphersIana);

At line 149, because the length of ctxAttr.list is greater than 0, ciphersIana is set to WolfSSL.getCiphersAvailableIana(this.currentVersion) from above:

        if(ctxAttr.list != null && ctxAttr.list.length > 0) {
            ciphersIana = ctxAttr.list;
        } else {
            ciphersIana = WolfSSL.getCiphersIana();
        }

Last, at line 167, the SSLParameters cipher suites are set to ciphersIana:
params.setCipherSuites(WolfSSLUtil.sanitizeSuites(ciphersIana));
I think that this is why calling getSupportedSSLParameters() then returns an SSLParameters object with only the list of "enabled" cipher suites and why the cipher list needed to be set again in getSupportedSSLParameters() in order to give the full list. Given all that, do you think something else should be changed in order to return the full list of ciphers in getSupportedSSLParameters()?
It does seem like setting the protocols again in supportedParams like I have is redundant though and I can delete that part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Javadocs for SSLContext.getSupportedSSLParameters(), it says:

Returns a copy of the SSLParameters indicating the supported settings for this SSL context.

My interpretation of "supported settings for this SSL context" means only cipher suites that are supported by this SSLContext object, which has been created with a specific TLS protocol version. Not all cipher suites compiled into native wolfSSL are supported by all protocol versions. For example, TLS 1.3 cipher suites are not supported in a TLS 1.2 SSLContext.

WolfSSL.getCiphersAvailableIana() returns the cipher suite list "supported" by the protocol version given to it. WolfSSL.getCiphersIana() returns the list of ALL cipher suites enabled at compile time in native wolfSSL. We want to return a filtered version of WolfSSL.getCiphersAvailableIana() as the "supported" cipher suites by this SSLContext.

More specifically, "supported" cipher suites start with the value of WolfSSL.getCiphersAvailableIana() but then that list may be filtered / restricted down to a subset by the following steps:

  1. We get full "supported" list of cipher suites from wolfSSL for the protocol version our SSLContext was created from (WolfSSL.getCiphersAvailableIana()).
  2. Some customers want to force restrict that list by hard-coding changes inside the WolfSSLCustomUser class. By default, we pass the list from step 1 into our default implementation of WolfSSLCustomUser. It does nothing to the list and returns it as the list to use.
  3. Users may further restrict the supported/enabled cipher suites by setting the wolfjsse.enabledCipherSuites Security property in their java.security file.
  4. We finally set the resulting cipher suite list into the SSLParameters:
171         params.setCipherSuites(WolfSSLUtil.sanitizeSuites(ciphersIana));

All that to say, I think we are setting the SSLParameters cipher suites to the correct "supported" list for a SSLContext object. And, the params we return from getSupportedSSLParameters() should be accurate already.

Let me know if you think this is still incorrect, thanks.

@cconlon cconlon assigned sstefonic and unassigned cconlon Jul 24, 2024
@sstefonic sstefonic requested a review from cconlon July 24, 2024 22:40
@cconlon cconlon removed their assignment Aug 7, 2024
@sstefonic sstefonic closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants