Skip to content

tls_codec: add variable-length integer type TlsVarInt#1656

Merged
franziskuskiefer merged 10 commits intoRustCrypto:masterfrom
boxdot:varint
Feb 19, 2026
Merged

tls_codec: add variable-length integer type TlsVarInt#1656
franziskuskiefer merged 10 commits intoRustCrypto:masterfrom
boxdot:varint

Conversation

@boxdot
Copy link
Contributor

@boxdot boxdot commented Feb 13, 2025

As defined in #rfc9000.

Also use this type (with an internal thin wrapper ContentLength) when
encoding/deconding the content length of vectors.

As defined in #[rfc9000].

Also use this type (with an internal thin wrapper `ContentLength`) when
encoding/deconding the content length of vectors.

[rfc9000]: https://www.rfc-editor.org/rfc/rfc9000#name-variable-length-integer-enc
@franziskuskiefer franziskuskiefer self-requested a review February 17, 2025 08:35
@franziskuskiefer
Copy link
Contributor

This totally fell of my radar. Sorry for that. I'll look at this soon.

Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Can you say a little more about what this PR is supposed to achieve? I struggle a little with understanding what it tries to do beyond refactoring. The variable length encoding is really only internal to the TLS encoding.

@boxdot
Copy link
Contributor Author

boxdot commented Feb 9, 2026

Can you say a little more about what this PR is supposed to achieve? I struggle a little with understanding what it tries to do beyond refactoring. The variable length encoding is really only internal to the TLS encoding.

The idea was to expose an additional type in the codec, to allow encoding/decoding of variable length integers. This is especially useful when prefixing small TLS encoded structs with such integers.

However, personally I stopped using this type, so if there no need for it for the TLS codec users, we can also close this PR. WDYT?

@franziskuskiefer
Copy link
Contributor

However, personally I stopped using this type, so if there no need for it for the TLS codec users, we can also close this PR. WDYT?

Thanks for clarifying.
I think there's some nice refactoring in here. I just wasn't sure about the purpose of making it public.
But encapsulating the variable length encoding looks much nicer.

@franziskuskiefer
Copy link
Contributor

@boxdot do you want to address the comments? I'd be happy to get this in then (with public or non-public varint, if it's something that's useful sometimes, there's no harm in exposing it).

@boxdot
Copy link
Contributor Author

boxdot commented Feb 17, 2026

@boxdot do you want to address the comments? I'd be happy to get this in then (with public or non-public varint, if it's something that's useful sometimes, there's no harm in exposing it).

I addressed review comments. We can do another review round.

@franziskuskiefer franziskuskiefer self-requested a review February 17, 2026 12:57
Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Great, thanks!
One nit, and can you add a line to the CHANGELOG for the new public TlsVarInt type?

}

fn check_min_len(value: u64, len: usize) -> Result<(), Error> {
if cfg!(feature = "mls") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be compile time, rather than a runtime check, or do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Changed.

Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks!

@franziskuskiefer franziskuskiefer merged commit e2c4c0b into RustCrypto:master Feb 19, 2026
13 checks passed
@boxdot
Copy link
Contributor Author

boxdot commented Feb 19, 2026

@franziskuskiefer Thank you for the review!

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.

3 participants