Add delegated credentials, as seen in #26 #28

claucece · 2020-08-05T08:02:09Z

For #26

src/crypto/tls/delegated_credentials.go

armfazh

Some general comments. @claucece

src/crypto/tls/delegated_credentials.go

src/crypto/tls/delegated_credentials_test.go

src/crypto/tls/delegated_credentials.go

src/crypto/tls/handshake_server_tls13.go

claucece · 2020-08-06T13:06:15Z

thanks @armfazh for the review! This is still WIP and has still some todos, so there is lots of errors. I'll assign you and @bwesterb once it is ready for review ;)

armfazh · 2020-08-06T18:18:05Z

This is still WIP and has still some todos, so there is lots of errors.

Yes, we are iterating in this codebase, and happy to review as many times as needed.

claucece · 2020-08-06T19:20:04Z

Yes, we are iterating in this codebase, and happy to review as many times as needed.

Oh, perfect! Every now and then when I finish a task of the issue, I'll mark it for review ;) Thanks so much!

thomwiggers · 2020-08-18T13:15:53Z

this should probably be rebased on the cf branch, because right now the diff is enormous

claucece · 2020-08-18T14:48:13Z

@thomwiggers the idea to have a separate branch is to be able to integrate to golang (which won't integrate probably the KEM TLS exp): that is why it differs from the cf branch.

claucece · 2020-08-19T14:58:09Z

@bwesterb @armfazh @chris-wood ready for review as a first version. I might add some refactors and other ideas in the mean time; but should we good now. Sorry for opening and closing it as a draft but I had some conflicts with go1.15

claucece · 2020-08-20T06:50:33Z

@bwesterb @armfazh Please, review until commit 8946da6

I'll add some more refactors, and some more functionality for the client; but I wanted you to check the overall work until that point ;)

armfazh

few comments

src/crypto/tls/delegated_credentials_test.go

src/crypto/tls/delegated_credentials.go

src/crypto/x509/x509.go

src/crypto/tls/handshake_client_tls13.go

bwesterb

I'm not intimately familiar with the Go Tls implementation nor with delegated credentials, so this "approve" should be read as a "I didn't see anything fishy". Looks good, well documented.

src/crypto/tls/common.go

src/crypto/tls/delegated_credentials.go

Lekensteyn

Initial (partial) review, I have yet to look at delegated_credentials.go and delegated_credentials_test.go

src/crypto/tls/common.go

src/crypto/tls/conn.go

src/crypto/tls/generate_cert.go

src/crypto/tls/handshake_client.go

src/crypto/tls/handshake_client_tls13.go

src/crypto/tls/handshake_messages.go

src/crypto/tls/handshake_server_tls13.go

src/crypto/tls/delegated_credentials_test.go

chris-wood

This is looking good! I left some comments for areas and things that might warrant further investigation. Please let me know if anything is not clear. :-)

src/crypto/tls/common.go

src/crypto/tls/delegated_credentials.go

src/crypto/tls/handshake_client_tls13.go

src/crypto/tls/delegated_credentials.go

src/crypto/tls/delegated_credentials_test.go

src/crypto/tls/delegated_credentials.go

src/crypto/tls/handshake_client_tls13.go

cjpatton

First round! Some high level things:

Unless there's a good reason to keep it, I think the GetDelegatedCredential callback should be replaced with a list of DCs. See inline comment for details.
Can you modify the package comment at the top of tls.go to document the fact that this package implements draft-ietf-tls-subcerts-09?
Please make sure all comments are wrapped at 80 characters.
For data serialization, use "golang.org/x/crypto/cryptobyte". This make the code much easier to read and verify.
Please remove any empty lines you added and add back any you removed. One of our goals for this repo is to minimize differences with upstream Go wherever possible

src/crypto/tls/common.go

src/crypto/tls/auth.go

src/crypto/tls/common.go

src/crypto/tls/handshake_messages.go

src/crypto/tls/handshake_server_tls13.go

claucece · 2021-01-06T13:36:54Z

Hi @cjpatton ! Thanks for this! I'll only solve this week the small comments, as any large change will likely break the code of kem-tls needed for Friday. Thanks!

cjpatton · 2021-01-27T17:17:46Z

Rebased.

wbl

A few comments that need addressing first

src/crypto/tls/tls_test.go

src/crypto/x509/verify_test.go

src/crypto/x509/x509.go

cjpatton

Last round for me. Just minor comments left.

src/crypto/tls/auth.go

src/crypto/tls/delegated_credentials.go

cjpatton · 2021-02-28T00:57:23Z

src/crypto/tls/common.go

+	// with TLS 1.3. If this is nil, then the server will not offer
+	// a DelegatedCredential. If the call returns nil, the server is also
+	// not offering a DelegatedCredential.
+	GetDelegatedCredential func(*ClientHelloInfo, *CertificateRequestInfo) (*DelegatedCredential, crypto.PrivateKey, error)


I'm good with whichever way you land here. (Consider this comment non-blocking.)

src/crypto/tls/handshake_messages.go

src/crypto/tls/tls.go

cjpatton

Unrequesting changes in order to unblock merge :)

claucece · 2021-03-01T21:03:27Z

Closing this, as it will only be experimental.

cjpatton · 2021-03-03T16:58:50Z

Rebased. Merging as-is, with existing approvals. Note that future changes to crypto/tls and crypto/x509 will need approval from the respective codeowners.

armfazh added the pqxp PostQuantum Experimentation label Aug 5, 2020

armfazh reviewed Aug 5, 2020

View reviewed changes

src/crypto/tls/delegated_credentials.go Outdated Show resolved Hide resolved

armfazh reviewed Aug 5, 2020

View reviewed changes

claucece marked this pull request as draft August 6, 2020 13:15

bwesterb force-pushed the cf branch 2 times, most recently from a82dde8 to 9d69f90 Compare August 12, 2020 08:49

claucece marked this pull request as ready for review August 19, 2020 11:57

claucece requested a review from chris-wood August 19, 2020 11:57

claucece changed the base branch from cf to go-master August 19, 2020 12:37

claucece marked this pull request as draft August 19, 2020 13:48

claucece force-pushed the cf-delegated-credentials branch from 359046f to b6252ee Compare August 19, 2020 14:55

claucece marked this pull request as ready for review August 19, 2020 14:56

claucece requested review from armfazh and bwesterb September 1, 2020 03:29

armfazh reviewed Sep 1, 2020

View reviewed changes

bwesterb approved these changes Sep 2, 2020

View reviewed changes

src/crypto/tls/common.go Outdated Show resolved Hide resolved

src/crypto/tls/delegated_credentials.go Outdated Show resolved Hide resolved

src/crypto/tls/delegated_credentials.go Outdated Show resolved Hide resolved

Lekensteyn reviewed Sep 2, 2020

View reviewed changes

src/crypto/tls/delegated_credentials_test.go Outdated Show resolved Hide resolved

chris-wood suggested changes Sep 2, 2020

View reviewed changes

cjpatton reviewed Sep 9, 2020

View reviewed changes

src/crypto/tls/delegated_credentials.go Outdated Show resolved Hide resolved

thomwiggers reviewed Sep 28, 2020

View reviewed changes

src/crypto/tls/delegated_credentials_test.go Outdated Show resolved Hide resolved

thomwiggers reviewed Sep 28, 2020

View reviewed changes

src/crypto/tls/delegated_credentials.go Outdated Show resolved Hide resolved

thomwiggers reviewed Sep 28, 2020

View reviewed changes

src/crypto/tls/handshake_client_tls13.go Show resolved Hide resolved

thomwiggers approved these changes Dec 9, 2020

View reviewed changes

thomwiggers mentioned this pull request Dec 15, 2020

Implement KEM TLS #44

Closed

claucece force-pushed the cf-delegated-credentials branch from 6b6a8f5 to 996ba2f Compare December 21, 2020 14:58

claucece force-pushed the cf-delegated-credentials branch from 996ba2f to ff75370 Compare December 30, 2020 22:26

cjpatton requested changes Jan 5, 2021

View reviewed changes

claucece force-pushed the cf-delegated-credentials branch from bdee57d to 2aab885 Compare January 7, 2021 19:05

cjpatton force-pushed the cf-delegated-credentials branch from f5589e0 to fa58fea Compare January 27, 2021 17:17

cjpatton mentioned this pull request Feb 22, 2021

Copyright for new files #54

Open

claucece force-pushed the cf-delegated-credentials branch 2 times, most recently from 8237cce to 3cd5592 Compare February 26, 2021 17:55

wbl approved these changes Feb 26, 2021

View reviewed changes

src/crypto/tls/tls_test.go Show resolved Hide resolved

src/crypto/x509/verify_test.go Show resolved Hide resolved

src/crypto/x509/x509.go Outdated Show resolved Hide resolved

wbl approved these changes Feb 27, 2021

View reviewed changes

cjpatton requested changes Feb 28, 2021

View reviewed changes

cjpatton reviewed Mar 1, 2021

View reviewed changes

cjpatton self-requested a review March 1, 2021 18:53

claucece force-pushed the cf-delegated-credentials branch from ebe950e to 4b94b48 Compare March 1, 2021 20:13

claucece closed this Mar 1, 2021

wbl reopened this Mar 2, 2021

crypto: Implement draft-ietf-tls-subcerts-10

3615e61

cjpatton force-pushed the cf-delegated-credentials branch from 4b94b48 to 3615e61 Compare March 3, 2021 16:57

cjpatton changed the base branch from cf to cf1.16 March 3, 2021 16:59

cjpatton merged commit d7473e7 into cf1.16 Mar 3, 2021

cjpatton deleted the cf-delegated-credentials branch March 3, 2021 17:47

cjpatton restored the cf-delegated-credentials branch March 3, 2021 17:47

bwesterb deleted the cf-delegated-credentials branch August 4, 2022 10:55

hslatman mentioned this pull request Jul 26, 2023

proposal: crypto/tls: add support for delegated credentials golang/go#35311

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delegated credentials, as seen in #26 #28

Add delegated credentials, as seen in #26 #28

claucece commented Aug 5, 2020 •

edited

Loading

armfazh left a comment

claucece commented Aug 6, 2020

armfazh commented Aug 6, 2020

claucece commented Aug 6, 2020

thomwiggers commented Aug 18, 2020

claucece commented Aug 18, 2020 •

edited

Loading

claucece commented Aug 19, 2020

claucece commented Aug 20, 2020

armfazh left a comment

bwesterb left a comment •

edited

Loading

Lekensteyn left a comment

chris-wood left a comment

cjpatton left a comment

claucece commented Jan 6, 2021

cjpatton commented Jan 27, 2021

wbl left a comment

cjpatton left a comment

cjpatton Feb 28, 2021

cjpatton left a comment

claucece commented Mar 1, 2021

cjpatton commented Mar 3, 2021 •

edited

Loading

Add delegated credentials, as seen in #26 #28

Add delegated credentials, as seen in #26 #28

Conversation

claucece commented Aug 5, 2020 • edited Loading

armfazh left a comment

Choose a reason for hiding this comment

claucece commented Aug 6, 2020

armfazh commented Aug 6, 2020

claucece commented Aug 6, 2020

thomwiggers commented Aug 18, 2020

claucece commented Aug 18, 2020 • edited Loading

claucece commented Aug 19, 2020

claucece commented Aug 20, 2020

armfazh left a comment

Choose a reason for hiding this comment

bwesterb left a comment • edited Loading

Choose a reason for hiding this comment

Lekensteyn left a comment

Choose a reason for hiding this comment

chris-wood left a comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

claucece commented Jan 6, 2021

cjpatton commented Jan 27, 2021

wbl left a comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

cjpatton Feb 28, 2021

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

claucece commented Mar 1, 2021

cjpatton commented Mar 3, 2021 • edited Loading

claucece commented Aug 5, 2020 •

edited

Loading

claucece commented Aug 18, 2020 •

edited

Loading

bwesterb left a comment •

edited

Loading

cjpatton commented Mar 3, 2021 •

edited

Loading