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.

Sign up for GitHub

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

Jump to bottom

[System]: Cleanup SslStream callbacks and internal validation code. #8753

Merged
merged 1 commit into from
May 18, 2018
Merged

[System]: Cleanup SslStream callbacks and internal validation code. #8753

merged 1 commit into from
May 18, 2018

Conversation

baulig
Copy link
Contributor

@baulig baulig commented May 17, 2018

  • SslStream: Reject attempts of setting conflicting callbacks using both the
    Mono-specific MonoTlsSettings and the new SslClientAuthenticationOptions /
    SslServerAuthenticationOptions.

    This makes it consistent with CoreFx behavior where those callbacks may only be
    specified in one of the possible places.

  • ChainValidationHelper - this internal class has received a major overhaul and
    lots of old and unused code removed.

    All callbacks are not invoked with the correct sender parameter to make it
    match the .NET / CoreFx behavior.

  • Mono.Security.Interface.CertificateValidationHelper: remove unused internal code.

[System]: Cleanup SslStream callbacks and internal validation code.
2ece36a 
* `SslStream`: Reject attempts of setting conflicting callbacks using both the
  Mono-specific `MonoTlsSettings` and the new `SslClientAuthenticationOptions` /
  `SslServerAuthenticationOptions`.

  This makes it consistent with CoreFx behavior where those callbacks may only be
  specified in one of the possible places.

* `ChainValidationHelper` - this internal class has received a major overhaul and
  lots of old and unused code removed.

  All callbacks are not invoked with the correct `sender` parameter to make it
  match the .NET / CoreFx behavior.

* `Mono.Security.Interface.CertificateValidationHelper`: remove unused internal code.
@baulig baulig self-assigned this May 17, 2018
baulig pushed a commit to baulig/mono that referenced this pull request May 18, 2018
[System]: Epic: Client Certificate Support - Part Two.
0208e77 
This is the second and final part to bring Client Certificate support.
It needs to be landed on top of mono#8753 and mono#8756.

* `Mono.Security.Interface.IMonoSslStream`: Add `CanRenegotiate` and `RenegotiateAsync()`.

* `Mono.Security.Interface.MonoTlsSettings`: Add `DisallowUnauthenticatedCertificateRequest`.

* `AppleTlsContext`: fully support renegotiation.
  - we may now receive `SslStatus.PeerAuthCompleted` and `SslStatus.PeerClientCertRequested`
    during `Read()`.  It should in theory not happen during `Write()`, but I added it there
    as well just to be on the safe side.
  - `SetSessionOption()` may only be called before the initial handshake.

* `MobileAuthenticatedStream`: this is the major part of the work and the most complex one.
  - added a new `Operation` enum to keep track of what is going on and detect invalid state.
  - a renegotion may only be triggered while we're idle - that is no handshake, read or write
    operation is currently active.
  - `InternalWrite()` may now be called from `SSLRead()`, the new `Operation` tells us what
    is currently happening.
  - `ProcessHandshake()` now takes a `bool renegotiate` argument.
  - added sanity checks to `ProcessRead()` and `ProcessWrite()`.

* `MobileTlsContext.SelectClientCertificate()`: check for
  `MonoTlsSettings.DisallowUnauthenticatedCertificateRequest`

* `MonoTlsProviderFactory.InternalVersion`: bump the internal version number.

Tests have already been added to `web-tests/master`, they will auto-enable themselves when
using a Mono runtime that contains this code.
@baulig baulig mentioned this pull request May 18, 2018
Merged
migueldeicaza
migueldeicaza reviewed May 18, 2018
View reviewed changes
mcs/class/System/Mono.Net.Security/ChainValidationHelper.cs
@@ -66,83 +66,43 @@ namespace Mono.Net.Security

internal class ChainValidationHelper : ICertificateValidator2
{
readonly object sender;
readonly WeakReference<SslStream> owner;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, is this because there might be unmanaged state and we do not want to create a strong cycle, or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was simply worried about creating a cycle, considering the memory leaks that we had in the past.

@baulig baulig merged commit 0429856 into mono:master May 18, 2018
@baulig baulig deleted the work-sslstream-updates branch May 18, 2018 16:35
baulig pushed a commit to baulig/mono that referenced this pull request May 24, 2018
[System]: Epic: Client Certificate Support - Part Two.
c51774a 
This is the second and final part to bring Client Certificate support.
It needs to be landed on top of mono#8753 and mono#8756.

* `Mono.Security.Interface.IMonoSslStream`: Add `CanRenegotiate` and `RenegotiateAsync()`.

* `Mono.Security.Interface.MonoTlsSettings`: Add `DisallowUnauthenticatedCertificateRequest`.

* `AppleTlsContext`: fully support renegotiation.
  - we may now receive `SslStatus.PeerAuthCompleted` and `SslStatus.PeerClientCertRequested`
    during `Read()`.  It should in theory not happen during `Write()`, but I added it there
    as well just to be on the safe side.
  - `SetSessionOption()` may only be called before the initial handshake.

* `MobileAuthenticatedStream`: this is the major part of the work and the most complex one.
  - added a new `Operation` enum to keep track of what is going on and detect invalid state.
  - a renegotion may only be triggered while we're idle - that is no handshake, read or write
    operation is currently active.
  - `InternalWrite()` may now be called from `SSLRead()`, the new `Operation` tells us what
    is currently happening.
  - `ProcessHandshake()` now takes a `bool renegotiate` argument.
  - added sanity checks to `ProcessRead()` and `ProcessWrite()`.

* `MobileTlsContext.SelectClientCertificate()`: check for
  `MonoTlsSettings.DisallowUnauthenticatedCertificateRequest`

* `MonoTlsProviderFactory.InternalVersion`: bump the internal version number.

Tests have already been added to `web-tests/master`, they will auto-enable themselves when
using a Mono runtime that contains this code.
baulig pushed a commit to baulig/mono that referenced this pull request May 24, 2018
[System]: Epic: Client Certificate Support - Part Two.
0a43e1c 
This is the second and final part to bring Client Certificate support.
It needs to be landed on top of mono#8753 and mono#8756.

* `Mono.Security.Interface.IMonoSslStream`: Add `CanRenegotiate` and `RenegotiateAsync()`.

* `Mono.Security.Interface.MonoTlsSettings`: Add `DisallowUnauthenticatedCertificateRequest`.

* `AppleTlsContext`: fully support renegotiation.
  - we may now receive `SslStatus.PeerAuthCompleted` and `SslStatus.PeerClientCertRequested`
    during `Read()`.  It should in theory not happen during `Write()`, but I added it there
    as well just to be on the safe side.
  - `SetSessionOption()` may only be called before the initial handshake.

* `MobileAuthenticatedStream`: this is the major part of the work and the most complex one.
  - added a new `Operation` enum to keep track of what is going on and detect invalid state.
  - a renegotion may only be triggered while we're idle - that is no handshake, read or write
    operation is currently active.
  - `InternalWrite()` may now be called from `SSLRead()`, the new `Operation` tells us what
    is currently happening.
  - `ProcessHandshake()` now takes a `bool renegotiate` argument.
  - added sanity checks to `ProcessRead()` and `ProcessWrite()`.

* `MobileTlsContext.SelectClientCertificate()`: check for
  `MonoTlsSettings.DisallowUnauthenticatedCertificateRequest`

* `MonoTlsProviderFactory.InternalVersion`: bump the internal version number.

Tests have already been added to `web-tests/master`, they will auto-enable themselves when
using a Mono runtime that contains this code.
marek-safar pushed a commit that referenced this pull request May 25, 2018
@marek-safar
[System]: Epic: Client Certificate Support - Part Two.
5715aee 
This is the second and final part to bring Client Certificate support.
It needs to be landed on top of #8753 and #8756.

* `Mono.Security.Interface.IMonoSslStream`: Add `CanRenegotiate` and `RenegotiateAsync()`.

* `Mono.Security.Interface.MonoTlsSettings`: Add `DisallowUnauthenticatedCertificateRequest`.

* `AppleTlsContext`: fully support renegotiation.
  - we may now receive `SslStatus.PeerAuthCompleted` and `SslStatus.PeerClientCertRequested`
    during `Read()`.  It should in theory not happen during `Write()`, but I added it there
    as well just to be on the safe side.
  - `SetSessionOption()` may only be called before the initial handshake.

* `MobileAuthenticatedStream`: this is the major part of the work and the most complex one.
  - added a new `Operation` enum to keep track of what is going on and detect invalid state.
  - a renegotion may only be triggered while we're idle - that is no handshake, read or write
    operation is currently active.
  - `InternalWrite()` may now be called from `SSLRead()`, the new `Operation` tells us what
    is currently happening.
  - `ProcessHandshake()` now takes a `bool renegotiate` argument.
  - added sanity checks to `ProcessRead()` and `ProcessWrite()`.

* `MobileTlsContext.SelectClientCertificate()`: check for
  `MonoTlsSettings.DisallowUnauthenticatedCertificateRequest`

* `MonoTlsProviderFactory.InternalVersion`: bump the internal version number.

Tests have already been added to `web-tests/master`, they will auto-enable themselves when
using a Mono runtime that contains this code.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@marek-safar
[System]: Epic: Client Certificate Support - Part Two.
80dcc72 
This is the second and final part to bring Client Certificate support.
It needs to be landed on top of mono/mono#8753 and mono/mono#8756.

* `Mono.Security.Interface.IMonoSslStream`: Add `CanRenegotiate` and `RenegotiateAsync()`.

* `Mono.Security.Interface.MonoTlsSettings`: Add `DisallowUnauthenticatedCertificateRequest`.

* `AppleTlsContext`: fully support renegotiation.
  - we may now receive `SslStatus.PeerAuthCompleted` and `SslStatus.PeerClientCertRequested`
    during `Read()`.  It should in theory not happen during `Write()`, but I added it there
    as well just to be on the safe side.
  - `SetSessionOption()` may only be called before the initial handshake.

* `MobileAuthenticatedStream`: this is the major part of the work and the most complex one.
  - added a new `Operation` enum to keep track of what is going on and detect invalid state.
  - a renegotion may only be triggered while we're idle - that is no handshake, read or write
    operation is currently active.
  - `InternalWrite()` may now be called from `SSLRead()`, the new `Operation` tells us what
    is currently happening.
  - `ProcessHandshake()` now takes a `bool renegotiate` argument.
  - added sanity checks to `ProcessRead()` and `ProcessWrite()`.

* `MobileTlsContext.SelectClientCertificate()`: check for
  `MonoTlsSettings.DisallowUnauthenticatedCertificateRequest`

* `MonoTlsProviderFactory.InternalVersion`: bump the internal version number.

Tests have already been added to `web-tests/master`, they will auto-enable themselves when
using a Mono runtime that contains this code.


Commit migrated from mono/mono@5715aee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@migueldeicaza migueldeicaza migueldeicaza approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@baulig baulig

Labels
area-BCL: System
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@baulig @migueldeicaza