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

ssl: Enhance client certificate selection #6204

Conversation

IngelaAndin
Copy link
Contributor

@IngelaAndin IngelaAndin commented Aug 9, 2022

In TLS-1.3 if chain certs are missing (so server auth domain adherence
can not be determined) send peer cert and hope the server is able to
recreate a chain in its auth domain.

Closes #6105

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

CT Test Results

No tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured.

Results for commit 27fdcf6

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Aug 9, 2022
@IngelaAndin IngelaAndin changed the base branch from master to maint August 9, 2022 12:26
@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Aug 9, 2022
end.

plausible_missing_chain([_] = EncodedChain, undefined, SignAlg, Key, Session0) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
plausible_missing_chain([_] = EncodedChain, undefined, SignAlg, Key, Session0) ->
plausible_missing_chain([_ | _] = EncodedChain, undefined, SignAlg, Key, Session0) ->

Does this need to be restricted to only one single cert in the EncodedChain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the assumption I made was that the case where the domain adherence check becomes a problem is when the client is "misconfigured" to only supply its own cert and not the other certs in the chain. As in this case, we cannot perform the check, but the server may be able to verify it anyway (TLS-1.3 spec mandates to try and do that). So in this case we don't really know and it is worth sending what we got. I could send a default always if the check fails, but that kind of makes the check pointless, and the TLS-1.3 spec also says the client SHOULD adhere to the server's auth domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjcarstens do you have an actual use case when only part of the chain is supplied? (Root cert can safely be left out).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe. We roughly follow the same setup for AWS IOT Client authentication using X.509 Certificates. The server authentication to complete handshake is the same - The device has the certificate chain and includes it in cacerts of the request. But the device/client also includes the signer CA used to sign it's device certificate as well which is typically a self-signed and managed CA and also put into the cacerts request.

This signer CA is not required by our server and is only used when the device certificate being presented is unknown (i.e. not pinned in our DB) and is required to be registered with our service first (also like AWS IOT requires registering CA's first). In that situation, we'll run PKIX validation with this signer CA and presented client cert before allowing the connection on the server. If we already know about the client certificate (after looking up its calculated pinned value in the DB), then the signer CA is simply ignored.

It's this extra, unchained signer CA that is getting us with the TLS 1.3 changes. I also don't claim to be an SSL/TLS expert, so it could be that I'm doing things completely wrong and don't fully understand the standards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, TLS-1.3 RFC says this about the extension:

The client MAY send the "certificate_authorities" extension in the
 ClientHello message.  The server MAY send it in the
 CertificateRequest message.
4.4.2.3.  Client Certificate Selection

   The following rules apply to certificates sent by the client:

   -  The certificate type MUST be X.509v3 [RFC5280], unless explicitly
      negotiated otherwise (e.g., [RFC7250]).

   -  If the "certificate_authorities" extension in the
      CertificateRequest message was present, at least one of the
      certificates in the certificate chain SHOULD be issued by one of
      the listed CAs.

   -  The certificates MUST be signed using an acceptable signature
      algorithm, as described in Section 4.3.2.  Note that this relaxes
      the constraints on certificate-signing algorithms found in prior
      versions of TLS.

   -  If the CertificateRequest message contained a non-empty
      "oid_filters" extension, the end-entity certificate MUST match the
      extension OIDs that are recognized by the client, as described in
      Section 4.2.5.

So I think that your use case fits better with having an option to disable the sending of the certificat_authorties extension.

In TLS 1.2 this particular extension is not present but an equivalent value exists sent in the certificate_reques, although the RFC is less clear on how to act, and before it was just ignored, so we let it be backward compatible.

@IngelaAndin IngelaAndin self-assigned this Aug 11, 2022
In TLS-1.3, if chain certs are missing (so server auth domain adherence
can not be determined) send peer cert and hope the server is able to
recreate a chain in its auth domain.

Closes erlang#6105
@IngelaAndin IngelaAndin force-pushed the ingela/ssl/client-certs/GH-6105/OTP-18191 branch from bc5e850 to 27fdcf6 Compare August 16, 2022 08:23
@IngelaAndin IngelaAndin merged commit 3071afd into erlang:maint Aug 16, 2022
jjcarstens added a commit to jjcarstens/otp that referenced this pull request Aug 17, 2022
…erify_peer`

Fixes erlang#6106

Based on the discussion from the issue (and erlang#6204), it was decided that adding the
ability to disable the `certificate_authorities` extension on the server would be
the fitting resolution for situations where you do not want that extension forced
in TLS 1.3.

This adds that ability to specify as a server option and defaults to `true` to keep
with existing expected functionality.
jjcarstens added a commit to jjcarstens/otp that referenced this pull request Sep 10, 2022
…erify_peer`

Fixes erlang#6106

Based on the discussion from the issue (and erlang#6204), it was decided that adding the
ability to disable the `certificate_authorities` extension on the server would be
the fitting resolution for situations where you do not want that extension forced
in TLS 1.3.

This adds that ability to specify as a server option and defaults to `true` to keep
with existing expected functionality.
jjcarstens added a commit to jjcarstens/otp that referenced this pull request Sep 11, 2022
…erify_peer`

Fixes erlang#6106

Based on the discussion from the issue (and erlang#6204), it was decided that adding the
ability to disable the `certificate_authorities` extension on the server would be
the fitting resolution for situations where you do not want that extension forced
in TLS 1.3.

This adds that ability to specify as a server option and defaults to `true` to keep
with existing expected functionality.
jjcarstens added a commit to jjcarstens/otp that referenced this pull request Sep 14, 2022
…erify_peer`

Fixes erlang#6106

Based on the discussion from the issue (and erlang#6204), it was decided that adding the
ability to disable the `certificate_authorities` extension on the server would be
the fitting resolution for situations where you do not want that extension forced
in TLS 1.3.

This adds that ability to specify as a server option and defaults to `true` to keep
with existing expected functionality.
jjcarstens added a commit to jjcarstens/otp that referenced this pull request Sep 23, 2022
…erify_peer`

Fixes erlang#6106

Based on the discussion from the issue (and erlang#6204), it was decided that adding the
ability to disable the `certificate_authorities` extension on the server would be
the fitting resolution for situations where you do not want that extension forced
in TLS 1.3.

This adds that ability to specify as a server option and defaults to `true` to keep
with existing expected functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS handshake issue
2 participants