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

RFC 9266: Channel Bindings for TLS 1.3 #4191 #4772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GuidoKiener
Copy link

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

Copy link
Contributor

@elliefm elliefm left a comment

Choose a reason for hiding this comment

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

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.

imap/tls.c Outdated Show resolved Hide resolved
imap/tls.c Outdated Show resolved Hide resolved
imtest/imtest.c Outdated Show resolved Hide resolved
@elliefm elliefm self-assigned this Dec 17, 2023
@elliefm elliefm added the backport-to-3.10 for PRs that are to be backported to 3.10 label Dec 17, 2023
@elliefm
Copy link
Contributor

elliefm commented Dec 18, 2023

@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
Copy link
Author

@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
Copy link
Contributor

elliefm commented Dec 18, 2023

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.

Copy link
Member

@rsto rsto left a comment

Choose a reason for hiding this comment

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

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

imap/tls.c Show resolved Hide resolved
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

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

@GuidoKiener GuidoKiener force-pushed the gk/exporter branch 2 times, most recently from cd0db98 to 0799088 Compare December 18, 2023 22:04
@GuidoKiener
Copy link
Author

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
Copy link

@GuidoKiener: A good job!

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

Linked to:

@Neustradamus
Copy link

@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:

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

Signed-off-by: Guido Kiener <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-3.10 for PRs that are to be backported to 3.10 Do Not Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants