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

Support renegotiation-based client auth. #111

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jun 26, 2019

Motivation:

While TLS renegotiation is in general a bad idea, it is widely used for
delayed TLS-based client authentication. We are able to support this
use-case, and so we should enable it.

As with BoringSSL, we'd like to continue to default to not supporting
this mode of operation. However, users are free to enable support if
they so choose.

Modifications:

  • Added flags for configuring renegotiation.
  • Added integration test harness.
  • Added renegotiation integration test with s_server.
  • Added renegotiation and client cert support to sample client.

Result:

Users will be able to perform TLS renegotiation.
Resolves #110

Motivation:

While TLS renegotiation is in general a bad idea, it is widely used for
delayed TLS-based client authentication. We are able to support this
use-case, and so we should enable it.

As with BoringSSL, we'd like to continue to default to not supporting
this mode of operation. However, users are free to enable support if
they so choose.

Modifications:

- Added flags for configuring renegotiation.
- Added integration test harness.
- Added renegotiation integration test with s_server.
- Added renegotiation and client cert support to sample client.

Result:

Users will be able to perform TLS renegotiation.
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jun 26, 2019
@Lukasa Lukasa added this to the 2.2.0 milestone Jun 26, 2019
@Lukasa Lukasa requested a review from weissi June 26, 2019 17:32
@Lukasa
Copy link
Contributor Author

Lukasa commented Jun 26, 2019

@weissi A note, we probably need to remember to actually run the integration tests.

/// Renegotiation is only supported in TLS 1.2 and earlier, and generally does not work very well. NIOSSL will
/// disallow most uses of renegotiation: the only supported use-case is to perform post-connection authentication
/// *as a client*. There is no way to initiate a TLS renegotiation in NIOSSL.
public enum NIORenegotiationSupport {
Copy link
Member

Choose a reason for hiding this comment

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

suggestions: Couldn't we nest this type in TLSConfiguration so ie would be TLSConfiguration.RenegotiationSupport? Then it doesn't need the NIO prefix either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I put the type here was to keep it in line with the other types used by TLSConfiguration, which are at the top level. If you think that consistency is less important than the convenience of nesting the type, I'm more than happy to move it: I don't really care either way.

Copy link
Member

Choose a reason for hiding this comment

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

@Lukasa I really don't mind either. Your call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave it where it is: status quo trumps convenience when it comes to the number of types we're discussing. I'll file an issue to move them all into the TLSConfiguration struct for v3.

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great. I'm basically happy with this, but would suggest to nest the NIORenegotiationSupport into the TLS config. If there are reasons not to do that, please feel free to dismiss

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@Lukasa Lukasa merged commit 06eea9a into apple:master Jun 26, 2019
@Lukasa Lukasa deleted the cb-allow-renegotiation branch June 26, 2019 20:08
normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Aug 17, 2021
Motivation:

renegiotion is not supported by BoringSSL by default but sometimes when using on the client-side you might want to support it for CLIENT-AUTH.
See also apple/swift-nio-ssl#111

Modifications:

Add native / java code to be able to enable support

Result:

Part of netty/netty#11529
normanmaurer added a commit to netty/netty-tcnative that referenced this pull request Aug 18, 2021
Motivation:

renegiotion is not supported by BoringSSL by default but sometimes when using on the client-side you might want to support it for CLIENT-AUTH.
See also apple/swift-nio-ssl#111

Modifications:

Add native / java code to be able to enable support

Result:

Part of netty/netty#11529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for allowing renegotiation
2 participants