RFC 9266: Channel Bindings for TLS 1.3 #4191

[GuidoKiener]

TLS connections of the IMAPD service provide channel binding data for the SASL authentication layer. The current implementation sets the correct "tls-unique" channel binding data for TLS versions 1.2 and lower, however not for TLS version 1.3.

TLS version 1.3 requires using specific exporter keying material (EKM) according to RFC 9266 Section 2:
Label: "EXPORTER-Channel-Binding"
Context: Zero-length string
Key Length: 32 bytes

[elliefm]

This looks okay to my eyes. I've approved the workflow so CI will now run, we'll see what that says...

There's a few places where the whitespace doesn't match our style. When we wrap function arguments, we try to wrap to the same column as the first argument. I've flagged these as "suggestions" inline, but I typed them in my browser, not my editor, so my suggestions might also be bad -- please do not apply them through the github web interface. If you're going to fix the whitespace, do it in your editor and amend/force push the commit.

[elliefm]

@ksmurchison @rsto Can you have a look at this as well please? It looks okay to my eyes, but SSL code warrants more eyes than just mine. There is more context here: #4191 (comment)

@GuidoKiener We're currently in feature freeze in preparation for the 3.10 release early next year, so I've marked this "Do Not Merge" for now. Assuming that it's approved eventually, I'll remove the label and merge it in the new year, after the feature freeze ends. We probably do want this in 3.10, but it's too late for it to sneak in, so I've also marked it for backporting once it lands on master.

[GuidoKiener]

@GuidoKiener We're currently in feature freeze in preparation for the 3.10 release early next year, so I've marked this "Do Not Merge" for now. Assuming that it's approved eventually, I'll remove the label and merge it in the new year, after the feature freeze ends. We probably do want this in 3.10, but it's too late for it to sneak in, so I've also marked it for backporting once it lands on master.

Ok, for me. It's just a contribution. I'll need another day to figure out how to fix and complete the merge request. Do you also have a code line limit e.g. 80 characters per line?

[elliefm]

I'll need another day to figure out how to fix and complete the merge request. Do you also have a code line limit e.g. 80 characters per line?

Yeah, 80 per line is our limit, though we don't always stick to it particularly strictly. The TLS update is useful enough that I'm not going to reject this PR just for whitespace problems, so if you don't get around to fixing the idents for whatever reason, I'll tidy that up myself later.

[rsto]

LGTM other than this could use a static_assert to catch issues at compile time. I'll leave final approval to @elliefm

[ksmurchison]

Other than the comments from my colleagues, looks good to me

[GuidoKiener]

I'll need another day to figure out how to fix and complete the merge request. Do you also have a code line limit e.g. 80 characters per line?

Yeah, 80 per line is our limit, though we don't always stick to it particularly strictly. The TLS update is useful enough that I'm not going to reject this PR just for whitespace problems, so if you don't get around to fixing the idents for whatever reason, I'll tidy that up myself later.

Sorry, I've fixed the code style issues after the changes have been approved by @ksmurchison. I hope it's ok for you to check the merge again.

[Neustradamus]

@GuidoKiener: A good job!

Like I have specified in the issue, tls-server-end-point is needed too:

• RFC 9266: Channel Bindings for TLS 1.3 support #4191

Linked to:

• State of Play scram-sasl/info#1

[Neustradamus]

@GuidoKiener: Can you look for "tls-server-end-point" at the same time?

It will be nice to be perfect in cyrus-sasl and cyrus-imapd:

• tls-unique for TLS =< 1.2
• tls-server-end-point
• tls-exporter for TLS = 1.3

Linked to:

• RFC 9266: Channel Bindings for TLS 1.3 support cyrus-sasl#742