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

[AndroidCrypto] Handle setting non-default SslProtocols #50987

Merged
merged 7 commits into from
Apr 10, 2021

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Apr 9, 2021

  • Set enabled SslProtocols
    • Throw PNSE if requested combination is not contiguous - matches macOS behaviour. Android actually does allow specifying a non-contiguous set, but it will only respect the lowest contiguous set and ignore the rest, so it seemed more consistent with our APIs to explicitly throw.
    • Ssl2 and Ssl3 are not supported, Tls13 is only supported on some (newer) versions
  • Always enlarge buffer when wrap/unwrap gives BUFFER_OVERFLOW
  • Fix some missed local ref deletes
  • Throw PNSE if EncryptionPolicy.NoEncryption is requested (not supported on Android)
  • Update test utilities to properly check for support for Ssl3 and Tls13 on Android

cc @jkoritzinsky @steveisok @AaronRobinsonMSFT @bartonjs @wfurt

@ghost
Copy link

ghost commented Apr 9, 2021

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Set enabled SslProtocols
    • Throw PNSE if requested combination is not contiguous - matches macOS behaviour. Android actually does allow specifying a non-contiguous set, but it will only respect the lowest contiguous set and ignore the rest, so it seemed more consistent with our APIs to explicitly throw.
    • Ssl2 is not supported, Ssl3 is only supported on some (older) versions, Tls13 is only supported on some (newer) versions
  • Always enlarge buffer when wrap/unwrap gives BUFFER_OVERFLOW
  • Fix some missed local ref deletes
  • Throw PNSE if EncryptionPolicy.NoEncryption is requested (not supported on Android)
  • Update test utilities to properly check for support for Ssl3 and Tls13 on Android
Author: elinor-fung
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@elinor-fung elinor-fung added this to the 6.0.0 milestone Apr 9, 2021
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Interop work looks good to me.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
I personally feel it would be ok to not worry about SSL2&3.
We drag them as legacy compat but they should be long dead.
Since Android probably does not have that baggage we may have more freedom.
Many Linux distributions do not supported them in practice as well.

@elinor-fung
Copy link
Member Author

I like that thought of not dragging along SSL3 as legacy compat - especially since the Android platform dropped support for it a couple of versions/years ago.

@elinor-fung elinor-fung merged commit 60698c7 into dotnet:main Apr 10, 2021
@elinor-fung elinor-fung deleted the sslProtocols-android branch April 10, 2021 17:45
@ghost ghost locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants