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

Add signature_algorithms_cert extension to _serverTLS13Handshake #513

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Odinmylord
Copy link

@Odinmylord Odinmylord commented Apr 9, 2024

At the moment the _serverTLS13Handshake function does not have a simple way to generate and send the signature_algorithms_cert extension. Since the extension is not generated by OpenSSL, tlslite-ng is the easiest way I found to create a webserver that sends this extension


This change is Reviewable

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Odinmylord)


-- commits line 2 at r1:
The point of the change is to include signature_algorithms_cert extension in the CertificateRequest, isn't it?


tlslite/handshakesettings.py line 400 at r1 (raw file):

        self.record_size_limit = 2**14 + 1  # TLS 1.3 includes content type
        # data needed for the signature algorithms cert extension
        self.more_sig_schemes_cert = []

they should have non empty default values; whether those should be the same as the ones for rsa, ecdsa, and other, or not is a separate question


tlslite/handshakesettings.py line 401 at r1 (raw file):

        # data needed for the signature algorithms cert extension
        self.more_sig_schemes_cert = []
        self.ecdsaSigHashesCert = []

camelCase is deprecated, it exists because I don't want to break API, new fields should use snake_case


tlslite/handshakesettings.py line 685 at r1 (raw file):

        other.more_sig_schemes_cert = self.more_sig_schemes_cert
        other.ecdsaSigHashesCert = self.ecdsaSigHashesCert
        other.rsaSigHashesCert = self.rsaSigHashesCert

the values need to be validated too


tlslite/tlsconnection.py line 2835 at r1 (raw file):

                cr_settings.more_sig_schemes = cr_settings.more_sig_schemes_cert
                cr_settings.ecdsaSigHashes = cr_settings.ecdsaSigHashesCert
                cr_settings.rsaSigHashes = cr_settings.rsaSigHashesCert

we definitely don't want to overwrite those values, settings object can be reused connection to connection

@tomato42
Copy link
Member

tomato42 commented Apr 9, 2024

(I'm assuming you want to be able to send different contents in the _cert and non cert extensions, so I've reviewed the PR as such, if that's not your intention, and you're fine with just duplicating values between them, then we don't need changes around handshake settings)

Copy link
Author

@Odinmylord Odinmylord left a comment

Choose a reason for hiding this comment

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

The idea is to have different values since if there is the same values, according to RFC8446, the signature_algorithms_cert extension can be omitted. Sorry for not being clear enough

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @tomato42)


-- commits line 2 at r1:

Previously, tomato42 (Hubert Kario) wrote…

The point of the change is to include signature_algorithms_cert extension in the CertificateRequest, isn't it?

Yeah, I could have been more clear, sorry


tlslite/handshakesettings.py line 400 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

they should have non empty default values; whether those should be the same as the ones for rsa, ecdsa, and other, or not is a separate question

Done. I put the same values as the "standard" extension since if the signature_algorithms_cert extension is not present the signature_algorithms extension is considered. I also updated the condition to not add the signature_algorithms_cert extension if it has the same values as signature_algorithms.


tlslite/handshakesettings.py line 401 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

camelCase is deprecated, it exists because I don't want to break API, new fields should use snake_case

Done.


tlslite/handshakesettings.py line 685 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

the values need to be validated too

I'm not sure I understand what you mean, isn't the validation performed by the _sigHashesToList function?


tlslite/tlsconnection.py line 2835 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

we definitely don't want to overwrite those values, settings object can be reused connection to connection

Done, also updated the _sigHashesToList function accordingly

@tomato42
Copy link
Member

CI failure are relevant

and privateKey and privateKey.n < 2**2047:
continue
# advertise support for both rsaEncryption and RSA-PSS OID
# key type
if certType != 'rsa-pss':
sigAlgs.append(getattr(SignatureScheme,
"rsa_{0}_rsae_{1}"
.format(schemeName, hashName)))
.format(scheme_name, hash_name)))
Copy link

Choose a reason for hiding this comment

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

Line too long (84 > 79 characters)

if certType != 'rsa':
sigAlgs.append(getattr(SignatureScheme,
"rsa_{0}_pss_{1}"
.format(schemeName, hashName)))
.format(scheme_name, hash_name)))
Copy link

Choose a reason for hiding this comment

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

Line too long (84 > 79 characters)

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.

None yet

2 participants