Delegated Credentials (WORK IN PROGRESS) #29

tsusanka · 2017-08-08T10:44:55Z

This is not yet finished, this PR is more of a "checkpoint".

TODO:

saving the credentials to use them until they're valid
test edge cases
TLS 1.3 support
incorporate into keyless

cc @grittygrease

grittygrease

This needs a major structural overhaul.

grittygrease · 2017-08-08T16:55:24Z

common.go

@@ -93,6 +94,7 @@ const (
 	extensionNextProtoNeg        uint16 = 13172 // not IANA assigned
 	extensionRenegotiationInfo   uint16 = 0xff01
 	extensionShortHeaders        uint16 = 0xff03 // Experimental
+	extensionDelegatedCredential uint16 = 99     // not IANA assigned yet - https://tools.ietf.org/html/draft-rescorla-tls-subcerts-01


Please use something in the 0xff experimental range for now.

grittygrease · 2017-08-08T17:23:28Z

common.go

@@ -614,6 +616,9 @@ type Config struct {
 	// this Config is returned by a GetConfigForClient callback. It's used
 	// by serverInit in order to copy session ticket keys if needed.
 	originalConfig *Config
+
+	// states if Delegated Credentials should be used
+	useDelegatedCredentials bool


This should be a public element so that a client can set its value when setting up a new TLS connection.

You're right, however, how come I still can access it? https://github.com/tsusanka/tls-tris/blob/tsusanka/delegated-cred/handshake_delegated_cred_test.go#L254

You can still access it because you're in the same package. However, external packages won't be able to set this value.

grittygrease · 2017-08-08T17:25:59Z

common.go

+func (hs *serverHandshakeState) isCertificateValidForDelegationUsage() bool {
+
+	for _, extension := range hs.cert.Leaf.Extensions {
+		if extension.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 99}) { // TODO: change this to constant?


Yes, this should be a constant.

grittygrease · 2017-08-08T17:37:28Z

handshake_client.go

+			hs.delegatedCredential = dc
+			// continue handshake with the credentials
+			// I'm still not sure this a good practise, it might be confusing
+			certs[0].PublicKey = dc.PublicKey


Probably not what you want to do. The logic below for checking the public key type should still check the public key. The delegated credential should be checked separately.

grittygrease · 2017-08-08T17:38:04Z

handshake_messages.go

-import "bytes"
+import (
+	"bytes"
+)


Not necessary

grittygrease · 2017-08-08T18:00:32Z

handshake_server.go

@@ -356,6 +369,24 @@ Curves:
 		}
 	}

+	if hs.clientHello.delegatedCredentials {


This logic should most likely be moved fully into GetCertificate. A cleaner design would be to simply create a new PrivateKey in the certificate object that corresponds to the delegated credential private key. That way almost all logic on the server side would be unchanged, aside from the addition of a new opaque delegated credentials object in the serverhello.

Thanks. I'm a little confused, how to use the GetCertificate function. If the c.GetCertificate is not null it is called to retrieve the certificate.

So, my goal is to implement a new GetCertificate function which I then "override" how the certificate is retrieved, right? The function will add the delegated credential and its PrivateKey. However, where this code should be? Also in the subpackage? Where do I then set the config to use this function?

Ah, alright, I simply set the function in the config when spinning up TLS, right? Similar to this

grittygrease · 2017-08-08T18:04:21Z

handshake_delegated_cred.go

+	"errors"
+	"time"
+)
+


This tight integration with the handshake is not ideal. I think all the credential minting and verifying should be abstracted into its own package (byte slice + cert to PrivateKey and back). The only pieces that need to change on the client side are the sending of the empty extension, the extraction and validation of the delegated credential, and the CertificateVerify function.

The only pieces that need to change on the client side are the sending of the empty extension, the extraction and validation of the delegated credential, and the CertificateVerify function.

I'm confused about the CertificateVerify part. Do you mean the CertificateVerify message? But that's used for client authentication, isn't it?

Actually, in TLS 1.2 this is called the ServerKeyExchange.

grittygrease · 2017-08-08T18:05:36Z

common.go

@@ -11,6 +11,7 @@ import (
 	"crypto/rand"
 	"crypto/sha512"
 	"crypto/x509"
+	"encoding/asn1"


As mentioned below, this is not a great dependency to add. Factor out as much logic as possible into its own package and try to keep the changes to crypto/tls minimal.

grittygrease · 2017-08-08T18:06:02Z

common.go

@@ -949,6 +954,9 @@ type Certificate struct {
 	// OCSPStaple contains an optional OCSP response which will be served
 	// to clients that request it.
 	OCSPStaple []byte
+	// DelegatedCredential contains a signed object containing a public
+	// key for signing handshakes
+	DelegatedCredential DelegatedCredential


This is probably the wrong place for this type of structure. When the Certificate is returned from GetCertificate it should already have a serialized opaque delegated credential, not a structure that needs to be serialized.

I understand. The Certificate struct will then contain marshalled DC and the associated PrivateKey. Both received from the delcred sub package.

Perhaps consider reusing the existing PrivateKey struct.

grittygrease · 2017-08-08T18:08:13Z

handshake_delegated_cred_test.go

@@ -0,0 +1,283 @@
+// Copyright 2010 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.


Again, this is too tightly integrated with the handshake. Each individual function should be unit tested with respect to its functionality, the internal tls structures should not be involved.

Okay. So I should add unit tests - e.g. check if the credential marshalled correctly etc. What is wrong with those higher-level tests though? They still provide value, don't they?

grittygrease · 2017-08-08T18:55:08Z

Config.Clone and other ancillary functions are also missing the new data.

grittygrease · 2017-08-08T18:55:45Z

Also note that the tests are failing.

tsusanka · 2017-08-09T11:21:48Z

Thanks for the review. I'm aware of the failing tests, it is some kind of race condition issue, I'll have a look into that once I fix the issues, you outlined above.

…ials Based on https://tools.ietf.org/html/draft-rescorla-tls-subcerts-01

WIP - this is not complete 1.3 support and is not yet tested

tsusanka · 2017-08-15T13:59:10Z

cipher_suites.go

@@ -33,7 +34,7 @@ type keyAgreement interface {

 	// This method may not be called if the server doesn't send a
 	// ServerKeyExchange message.
-	processServerKeyExchange(*Config, *clientHelloMsg, *serverHelloMsg, *x509.Certificate, *serverKeyExchangeMsg) error
+	processServerKeyExchange(*Config, *clientHelloMsg, *serverHelloMsg, crypto.PublicKey, *serverKeyExchangeMsg) error


It might be possible to avoid this change by wrapping the DC's PublicKey into an x509.Certificate. However:
a) I'm not sure it'll work since we have only the PublicKey
b) all processServerKeyExchange functions use only the public key, so the dependency for x509.Certificate seems redundant
c) the function is not exported, so it should not be a BC break (?)

This is a slick change. I can't see why it wouldn't work or why *x509.Certificate was used in the first place.

tsusanka · 2017-08-15T14:15:23Z

I think this is ready for another round of code review.

All code is in the same tls package, however, the logic is clearly separated using the GetCertificate function as discussed
I've squashed all commits into just a few, it was a mess
Tests are fixed
I've implemented the support for PKCS1, it's trivial to remove it in case we decide to

grittygrease · 2017-08-15T18:22:32Z

cipher_suites.go

@@ -33,7 +34,7 @@ type keyAgreement interface {

 	// This method may not be called if the server doesn't send a
 	// ServerKeyExchange message.
-	processServerKeyExchange(*Config, *clientHelloMsg, *serverHelloMsg, *x509.Certificate, *serverKeyExchangeMsg) error
+	processServerKeyExchange(*Config, *clientHelloMsg, *serverHelloMsg, crypto.PublicKey, *serverKeyExchangeMsg) error


This is a slick change. I can't see why it wouldn't work or why *x509.Certificate was used in the first place.

grittygrease · 2017-08-15T18:23:05Z

delegated_credentials.go

+}
+
+func NewDelCredGetCertificateFunction(cert *Certificate) GetCertificate {
+


Unnecessary space?

grittygrease · 2017-08-15T18:24:04Z

delegated_credentials.go

+	PublicKey interface{}
+}
+
+func NewDelCredGetCertificateFunction(cert *Certificate) GetCertificate {


This abbreviation is a bit awkward. NewGetCertificateFromDelegatedCredential? That's also long but a bit better.

Yeah, I don't really like it myself. I'm not sure the From is accurate though. So how about simply NewDelegatedCredentialGetCertificate?

grittygrease · 2017-08-15T18:24:27Z

delegated_credentials.go

+func NewDelCredGetCertificateFunction(cert *Certificate) GetCertificate {
+
+	return func(clientHelloInfo *ClientHelloInfo) (*Certificate, error) {
+


grittygrease · 2017-08-15T18:24:33Z

delegated_credentials.go

+}
+
+func selectVersion(versions []uint16) uint16 {
+


grittygrease · 2017-08-15T18:29:49Z

delegated_credentials.go

+	if scheme == ECDSAWithP256AndSHA256 {
+		privateKey, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+		credential.PublicKey = privateKey.(crypto.Signer).Public()
+


grittygrease · 2017-08-15T18:30:12Z

delegated_credentials.go

+
+	copy(cred[7:7+publicKeyLength], publicKeyBytes)
+
+	cred[publicKeyLength+7] = uint8(scheme >> 8) // hash


This will likely be different for TLS 1.3

grittygrease · 2017-08-15T18:35:27Z

handshake_client.go

+	finishedHash     finishedHash
+	masterSecret     []byte
+	session          *ClientSessionState
+	delCredPublicKey crypto.PublicKey


Maybe this should be just connection PublicKey, and it is set to either the delegated cred key or c.peerCertificates[0].PublicKey. Then you may be able to reduce the number of if statements below.

tsusanka · 2017-08-22T12:20:37Z

delegated_credentials.go

+)
+
+// After only renewThreshold of credential's validTime is left a new one is created
+const renewThreshold = 0.2


Maybe better to reverse this? Set it to 0.8 and call it renewAfterThreshold

…renewed using goroutine

grittygrease

The caching logic needs to be moved out of the global namespace.

grittygrease · 2017-08-23T01:43:45Z

delegated_credentials.go

+// SignatureScheme to ensure client's support.
+func createDelegatedCredential(config *CredentialsConfig) (credential DelegatedCredential, privateKey crypto.PrivateKey, renewAfter time.Time, err error) {
+	validTill := config.timeFunc().Add(config.Validity)
+	relativeToCert := validTill.Sub(config.Cert.Leaf.NotBefore)


The computation of the ValidTime is a rather complex dance. Is there a simpler way to compute it that is more readable?

Yeah, I agree. I was thinking about it and I'm not sure there is a better way though. The code is basically the definition in itself:

Relative time in seconds from the beginning of the certificate's notBefore value after which the Delegated Credential is no longer valid

Btw why is this relative? Why not making it a simple datetime when the credential expires? The client needs to verify the certificate's validity anyway.

I can remove the relativeToCert variable but it's not significantly better:

validTill := config.timeFunc().Add(config.Validity) renewAfter = validTill.Add(-time.Duration(config.Validity.Seconds()*renewThreshold) * time.Second) credential = DelegatedCredential{ ValidTime: int64(validTill.Sub(config.Cert.Leaf.NotBefore).Seconds()), }

grittygrease · 2017-08-23T01:50:44Z

delegated_credentials.go

+	renewChan chan struct{}
+}
+
+var cachedCredentials map[SignatureScheme]*cachedCredential


This is a package-global variable indexed by the SignatureScheme. This means it will be shared between all instantiations, even those with different certs. You most likely want to keep this local to the CredentialsConfig.

Good point! So I'm just including it as a not exported CredentialsConfig variable.

grittygrease · 2017-08-23T01:55:38Z

delegated_credentials.go

+			go func() {
+				cachedCredentials[config.scheme], err = newCredential(config)
+				if config.renewChan != nil {
+					config.renewChan <- struct{}{}


Where is the renewChan read?

Only in tests. I was reluctant to add the channel just because of tests, but I wanted to test that the new credential is created, which I can do only after the goroutine finishes. I was talking to Salmen about this and he advised me this.

…structure

tsusanka · 2017-08-25T13:00:01Z

Closed in favour #32

grittygrease suggested changes Aug 8, 2017

View reviewed changes

tsusanka force-pushed the tsusanka/delegated-cred branch 3 times, most recently from 5e023e6 to acc9ee8 Compare August 15, 2017 12:54

tsusanka added 2 commits August 15, 2017 14:16

tris/DelegatedCredentials: basic TLS 1.2 support for DelegatedCredent…

28469d4

…ials Based on https://tools.ietf.org/html/draft-rescorla-tls-subcerts-01

tris/DelegatedCredentials: PKCS support; RSA handshake test

3df9bf9

tsusanka force-pushed the tsusanka/delegated-cred branch from acc9ee8 to 3df9bf9 Compare August 15, 2017 13:20

tris/DelegatedCredentials: TLS 1.3 messages

1de138d

WIP - this is not complete 1.3 support and is not yet tested

tsusanka commented Aug 15, 2017

View reviewed changes

grittygrease reviewed Aug 15, 2017

View reviewed changes

tsusanka added 3 commits August 16, 2017 10:22

tris/DelegatedCredentials: typos

37b5cfa

tris/DelegatedCredentials: serverPublicKey in clientHandshakeState

045ef9c

tris/DelegatedCredentials: caching

4884882

tsusanka force-pushed the tsusanka/delegated-cred branch 2 times, most recently from c853649 to 8bbddcd Compare August 22, 2017 10:55

tsusanka commented Aug 22, 2017

View reviewed changes

tsusanka added 2 commits August 22, 2017 13:28

tris/DelegatedCredentials: typos

5c54e17

tris/DelegatedCredentials: CredentialsConfig struct; credentials are …

94d5a8b

…renewed using goroutine

tsusanka force-pushed the tsusanka/delegated-cred branch from 8bbddcd to 94d5a8b Compare August 22, 2017 12:28

grittygrease suggested changes Aug 23, 2017

View reviewed changes

tris/DelegatedCredentials: credentials cache is a part of the Config …

97f7a5f

…structure

tsusanka closed this Aug 25, 2017

		}

		func NewDelCredGetCertificateFunction(cert *Certificate) GetCertificate {

		func NewDelCredGetCertificateFunction(cert *Certificate) GetCertificate {

		return func(clientHelloInfo ClientHelloInfo) (Certificate, error) {


		copy(cred[7:7+publicKeyLength], publicKeyBytes)

		cred[publicKeyLength+7] = uint8(scheme >> 8) // hash

Delegated Credentials (WORK IN PROGRESS) #29

Delegated Credentials (WORK IN PROGRESS) #29

Conversation

tsusanka commented Aug 8, 2017 • edited Loading

grittygrease left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grittygrease commented Aug 8, 2017

grittygrease commented Aug 8, 2017

tsusanka commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grittygrease left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka commented Aug 25, 2017

tsusanka commented Aug 8, 2017 •

edited

Loading

tsusanka Aug 9, 2017 •

edited

Loading

tsusanka commented Aug 9, 2017 •

edited

Loading

tsusanka Aug 15, 2017 •

edited

Loading