Define api for delegated credentials #67 #69

claucece · 2021-03-18T19:57:18Z

Ready for review.

This should solve #63 as well.

claucece · 2021-03-18T22:05:06Z

@armfazh @lukevalenta @Lekensteyn ready for review now. Thanks!

cjpatton

I like the API changes. One concern sticks out to me: If I'm understanding the code correctly, the client (resp. server) may use a DC whose signature algorithm isn't supported by the peer. I may be wrong :)

src/crypto/tls/common.go

src/crypto/tls/delegated_credentials_test.go

src/crypto/tls/handshake_client_tls13.go

src/crypto/tls/handshake_server_tls13.go

src/crypto/tls/handshake_client_tls13.go

claucece · 2021-03-19T20:30:17Z

@cjpatton addressed now. Let me know what you think now ;)

One concern sticks out to me: If I'm understanding the code correctly, the client (resp. server) may use a DC whose signature algorithm isn't supported by the peer. I may be wrong :)

When you get a DC from storage you check which one you will used based on the other's peer signature algorithm advertised by either the clientHelloInfo or certificateRequestInfo, so it will not be able to use anything that wasn't advertised there in the handshake.

Lekensteyn

If I understand correctly, the point of this change is to provide the DC in a single callback via GetCertificate rather than two (GetCertificate followed by GetDelegatedCredential)?

Could you include the motivation for the change in the commit message? Thanks, it looks good with minor comments!

src/crypto/tls/delegated_credentials_test.go

Lekensteyn · 2021-03-22T15:16:40Z

src/crypto/tls/delegated_credentials_test.go

@@ -446,7 +462,9 @@ func testServerGetCertificate(ch *ClientHelloInfo) (*Certificate, error) {
 	}

 	if versOk && ch.SupportsDelegatedCredential {
-		return dcTestCerts["dcP256"], nil
+		serverCert := dcTestCerts["dcP256"]
+		serverCert.DelegatedCredentials = serverDC[dcCount:]


(Note for my understanding: initDCTest resets dcTestCerts and dcTestCerts["dcCertP256"] which is why it is okay for testServerGetCertificate to have side-effects such as overwriting dcTestCerts["dcP256"].DelegatedCredentials.)

I was wondering why serverDC[dcCount:] was used. Would serverDC[dcCount:dcCount+1] be sufficient to limit the test to a specific DC? Or is there a reason to include more than one?

It is more for testing with each specific DC and algorithm (a DC with ed25519, a Dc with eddsa256, etc). I'll prefer if the tests were only using one DC (it simplifies all) but is was requested by reviewers to test with them all.

So based on a private chat with Sofía, my understanding is that it deliberately includes more than one to test this code path:

go/src/crypto/tls/handshake_server_tls13.go

Lines 464 to 472 in 1777b4c

for _, dcPair := range cert.DelegatedCredentials {

// The client must have sent the signature_algorithms in the DC extension: ensure it supports

// schemes we can use with this delegated credential.

if len(clientHello.SignatureSchemesDC) > 0 {

if _, err := selectSignatureSchemeDC(VersionTLS13, dcPair.DC, clientHello.SignatureSchemesDC); err == nil {

return &dcPair, nil

}

}

}

cjpatton

One final question from me: From the tests it appears as if a DC may still be used in the connection even if support for DCs was not indicated by the peer. Is this an artifact of the test code, or is this what's actually happening?

src/crypto/tls/delegated_credentials_test.go

src/crypto/tls/handshake_client_tls13.go

src/crypto/tls/handshake_server_tls13.go

src/crypto/tls/handshake_client_tls13.go

claucece · 2021-03-22T18:30:04Z

If I understand correctly, the point of this change is to provide the DC in a single callback via GetCertificate rather than two (GetCertificate followed by GetDelegatedCredential)?
Could you include the motivation for the change in the commit message? Thanks, it looks good with minor comments!

Yes, the idea is to unify into single callback. I'll include in the commit message ;)

cjpatton

LGTM

armfazh

few nits

src/crypto/tls/common.go

src/crypto/tls/handshake_server_tls13.go

Lekensteyn

With an updated commit message with the motivation, it looks good to me.

…using the same mechanisms used to fetch certificates #67 Refactor new API Address comments from review Address comments from review 2 Address comments from review 3

claucece · 2021-03-23T15:53:41Z

Commit message has been changed. Merging now.

…using the same mechanisms used to fetch certificates #67 (#69) Refactor new API Address comments from review Address comments from review 2 Address comments from review 3

crypto/tls: Implement draft-ietf-tls-subcerts-10 crypto/tls: fixes individual testing by adding insecure verify #62 crypto/tls: define api for delegated credentials so they are fetched using the same mechanisms used to fetch certificates #67 (#69) Refactor new API Address comments from review Address comments from review 2 Address comments from review 3 crypto/tls: allow the usage of other keyUsage when checking for the dc extension #72 (#73)

…using the same mechanisms used to fetch certificates #67 (#69) Refactor new API Address comments from review Address comments from review 2 Address comments from review 3

claucece requested a review from Lekensteyn as a code owner March 18, 2021 19:57

claucece requested review from armfazh and lukevalenta March 18, 2021 19:57

claucece marked this pull request as draft March 18, 2021 20:48

claucece force-pushed the dc-api branch from e14a9c5 to dc791fd Compare March 18, 2021 22:03

claucece marked this pull request as ready for review March 18, 2021 22:04

claucece requested a review from cjpatton March 18, 2021 22:14

claucece changed the title ~~[DRAFT] Define api for delegated credentials #67~~ Define api for delegated credentials #67 Mar 18, 2021

cjpatton reviewed Mar 19, 2021

View reviewed changes

cjpatton self-requested a review March 19, 2021 15:40

Lekensteyn reviewed Mar 22, 2021

View reviewed changes

cjpatton reviewed Mar 22, 2021

View reviewed changes

cjpatton approved these changes Mar 22, 2021

View reviewed changes

armfazh approved these changes Mar 22, 2021

View reviewed changes

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

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

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

Lekensteyn approved these changes Mar 23, 2021

View reviewed changes

crypto/tls: define api for delegated credentials so they are fecthed …

24d7180

…using the same mechanisms used to fetch certificates #67 Refactor new API Address comments from review Address comments from review 2 Address comments from review 3

claucece force-pushed the dc-api branch from 1777b4c to 24d7180 Compare March 23, 2021 15:43

claucece merged commit c5faf90 into cf Mar 23, 2021

claucece deleted the dc-api branch March 23, 2021 15:56

Lekensteyn mentioned this pull request Jan 14, 2022

Rebase on Go 1.17.6, add PQSignatureSchemesEnabled option #107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define api for delegated credentials #67 #69

Define api for delegated credentials #67 #69

claucece commented Mar 18, 2021 •

edited

Loading

claucece commented Mar 18, 2021

cjpatton left a comment

claucece commented Mar 19, 2021

Lekensteyn left a comment

Lekensteyn Mar 22, 2021

claucece Mar 22, 2021

Lekensteyn Mar 23, 2021 •

edited

Loading

cjpatton left a comment

claucece commented Mar 22, 2021

cjpatton left a comment

armfazh left a comment

Lekensteyn left a comment

claucece commented Mar 23, 2021

	for _, dcPair := range cert.DelegatedCredentials {
	// The client must have sent the signature_algorithms in the DC extension: ensure it supports
	// schemes we can use with this delegated credential.
	if len(clientHello.SignatureSchemesDC) > 0 {
	if _, err := selectSignatureSchemeDC(VersionTLS13, dcPair.DC, clientHello.SignatureSchemesDC); err == nil {
	return &dcPair, nil
	}
	}
	}

Define api for delegated credentials #67 #69

Define api for delegated credentials #67 #69

Conversation

claucece commented Mar 18, 2021 • edited Loading

claucece commented Mar 18, 2021

cjpatton left a comment

Choose a reason for hiding this comment

claucece commented Mar 19, 2021

Lekensteyn left a comment

Choose a reason for hiding this comment

Lekensteyn Mar 22, 2021

Choose a reason for hiding this comment

claucece Mar 22, 2021

Choose a reason for hiding this comment

Lekensteyn Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

claucece commented Mar 22, 2021

cjpatton left a comment

Choose a reason for hiding this comment

armfazh left a comment

Choose a reason for hiding this comment

Lekensteyn left a comment

Choose a reason for hiding this comment

claucece commented Mar 23, 2021

claucece commented Mar 18, 2021 •

edited

Loading

Lekensteyn Mar 23, 2021 •

edited

Loading