fix (registration): Use registration challenge in kms-led authorization

.gitignore

@@ -120,3 +120,5 @@ main
 /bin/
 
 .schema-diff/
+
+.env

registration/authorize.go

@@ -119,6 +119,14 @@ func authorizeNodeCommon(
 		WrappingRegistrationFlowInfo:     reqInfo.WrappingRegistrationFlowInfo,
 	}
 
+	// When WrappingRegistrationFlowInfo is present, in the new flow,
+	// a registration challenge will be present and needs to be copied to the resulting nodeInfo
+	if reqInfo.WrappingRegistrationFlowInfo != nil &&
+		reqInfo.WrappingRegistrationFlowInfo.RegistrationChallenge != nil &&
+		len(reqInfo.WrappingRegistrationFlowInfo.RegistrationChallenge.Challenge) != 0 {
+		nodeInfo.RegistrationChallenge = reqInfo.WrappingRegistrationFlowInfo.RegistrationChallenge
+	}
+
 	certPubKeyRaw, err := x509.ParsePKIXPublicKey(nodeInfo.CertificatePublicKeyPkix)
 	if err != nil {
 		return nil, fmt.Errorf("(%s) error parsing node certificate public key: %w", op, err)

registration/register_node_led.go

@@ -154,12 +154,38 @@ func FetchNodeCredentials(
 
 		// Regardless of how we got the decrypted data, perform validation here
 		// since here is where we validated the signature on the bundle and the
-		// overall validity of the bundle
-		if subtle.ConstantTimeCompare(registrationInfo.Nonce, reqInfo.Nonce) != 1 {
+		// overall validity of the bundle. Nonce shouldn't show up, but compare whatever is provided
+		if len(registrationInfo.Nonce) == 0 &&
+			(registrationInfo.RegistrationChallenge == nil || len(registrationInfo.RegistrationChallenge.Challenge) == 0) {
+			err := errors.New("missing either a nonce or registration challenge in wrapped registration info")
+			opts.WithLogger.Error(err.Error(), "op", op)
+			return nil, fmt.Errorf("(%s) %s", op, err.Error())
+
+		}
+
+		if len(registrationInfo.Nonce) != 0 && subtle.ConstantTimeCompare(registrationInfo.Nonce, reqInfo.Nonce) != 1 {
 			err := errors.New("mismatched nonce in unwrapped registration info")
 			opts.WithLogger.Error(err.Error(), "op", op)
 			return nil, fmt.Errorf("(%s) %s", op, err.Error())
 		}
+
+		if registrationInfo.RegistrationChallenge != nil &&
+			len(registrationInfo.RegistrationChallenge.Challenge) != 0 &&
+			subtle.ConstantTimeCompare(registrationInfo.RegistrationChallenge.Challenge, reqInfo.RegistrationChallenge.Challenge) != 1 {
+			err := errors.New("mismatched registration challenge in unwrapped registration info")
+			opts.WithLogger.Error(err.Error(), "op", op)
+			return nil, fmt.Errorf("(%s) %s", op, err.Error())
+
+		}
+
+		// Check if pub key is empty before comparing, only need to check one
+		// to avoid them both being empty and equal
+		if len(registrationInfo.CertificatePublicKeyPkix) == 0 {
+			err := errors.New("empty public key in unwrapped registration info")
+			opts.WithLogger.Error(err.Error(), "op", op)
+			return nil, fmt.Errorf("(%s) %s", op, err.Error())
+		}
+
 		if subtle.ConstantTimeCompare(registrationInfo.CertificatePublicKeyPkix, reqInfo.CertificatePublicKeyPkix) != 1 {
 			err := errors.New("mismatched public key in unwrapped registration info")
 			opts.WithLogger.Error(err.Error(), "op", op)
@@ -236,10 +262,16 @@ func FetchNodeCredentials(
 		}
 	}
 
+	if len(nodeInfo.CertificatePublicKeyPkix) == 0 {
+		return nil, fmt.Errorf("(%s) nodeInfo is missing public key pkix", op)
+	}
 	if subtle.ConstantTimeCompare(nodeInfo.CertificatePublicKeyPkix, reqInfo.CertificatePublicKeyPkix) != 1 {
 		return nil, fmt.Errorf("(%s) mismatched certificate public keys between authorization and incoming fetch request", op)
 	}
 
+	if len(nodeInfo.EncryptionPublicKeyBytes) == 0 {
+		return nil, fmt.Errorf("(%s) nodeInfo is missing public key bytes", op)
+	}
 	if subtle.ConstantTimeCompare(nodeInfo.EncryptionPublicKeyBytes, reqInfo.EncryptionPublicKeyBytes) != 1 {
 		return nil, fmt.Errorf("(%s) mismatched encryption public keys between authorization and incoming fetch request", op)
 	}
@@ -251,7 +283,7 @@ func FetchNodeCredentials(
 		CertificateBundles:             nodeInfo.CertificateBundles,
 	}
 
-	// If it's node-led activation and there's a challenge, ensure we include
+	// If it's node-led or kms-led activation and there's a challenge, ensure we include
 	// the encrypted challenge back. In server-led, the node provides the
 	// encrypted registration challenge.
 	if nodeInfo.RegistrationChallenge != nil && reqInfo.EncryptedRegistrationChallenge == nil {

registration/register_node_led_test.go

@@ -708,3 +708,54 @@ func TestNodeLedRegistration_FetchNodeCredentials(t *testing.T) {
 		})
 	}
 }
+
+// This test is similar to regular node-led registration but provides a registration wrapper
+func TestKmsLedRegistration(t *testing.T) {
+	t.Parallel()
+	assert := assert.New(t)
+	require := require.New(t)
+	ctx := context.Background()
+
+	// Functions using nodeStorage happen node-side
+	nodeStorage, err := inmem.New(ctx)
+	require.NoError(err)
+
+	// Functions using controllerStorage happen controller-side
+	controllerStorage, err := inmem.New(ctx)
+	require.NoError(err)
+
+	_, err = rotation.RotateRootCertificates(ctx, controllerStorage)
+	require.NoError(err)
+
+	originalNodeCreds, err := types.NewNodeCredentials(ctx, nodeStorage)
+	require.NoError(err)
+
+	registrationWrapper := wrapping.NewTestWrapper([]byte("emi wuz here"))
+
+	fetchReq, err := originalNodeCreds.CreateFetchNodeCredentialsRequest(ctx, nodeStorage,
+		nodeenrollment.WithoutRegistrationChallenge(true),
+		nodeenrollment.WithRegistrationWrapper(registrationWrapper),
+	)
+	require.NoError(err)
+
+	keyId, err := nodeenrollment.KeyIdFromPkix(originalNodeCreds.CertificatePublicKeyPkix)
+	require.NoError(err)
+
+	resp, err := registration.FetchNodeCredentials(t.Context(), controllerStorage, fetchReq, nodeenrollment.WithRegistrationWrapper(registrationWrapper))
+	require.NoError(err)
+	require.NotNil(resp)
+
+	checkNodeInfo := &types.NodeInformation{Id: keyId}
+	require.NotNil(resp.EncryptedNodeCredentials)
+	require.NotNil(resp.ServerEncryptionPublicKeyBytes)
+	require.Equal(types.KEYTYPE_X25519, resp.ServerEncryptionPublicKeyType)
+
+	require.NoError(controllerStorage.Load(ctx, checkNodeInfo))
+	require.NotNil(checkNodeInfo)
+
+	newCreds, err := originalNodeCreds.HandleFetchNodeCredentialsResponse(t.Context(), nodeStorage, resp)
+	require.NoError(err)
+	require.NotNil(newCreds)
+	assert.NotEmpty(newCreds.ServerEncryptionPublicKeyBytes)
+	assert.Equal(types.KEYTYPE_X25519, newCreds.ServerEncryptionPublicKeyType)
+}

tls/client.go

@@ -200,7 +200,7 @@ func ClientConfigs(ctx context.Context, n *types.NodeCredentials, opt ...nodeenr
 			for _, acceptableCa := range cri.AcceptableCAs {
 				// log.Println("GetClientCertificate", base64.RawStdEncoding.EncodeToString(acceptableCa))
 				for _, bundle := range certMap {
-					if subtle.ConstantTimeCompare(bundle.ca.RawSubject, acceptableCa) == 1 {
+					if len(bundle.ca.RawSubject) != 0 && subtle.ConstantTimeCompare(bundle.ca.RawSubject, acceptableCa) == 1 {
 						return &tls.Certificate{
 							Certificate: [][]byte{
 								bundle.leaf.Raw,

types/github.com.hashicorp.nodeenrollment.types.v1.proto

@@ -167,7 +167,7 @@ message NodeCredentials {
 // WrappingRegistrationFlowInfo is a message that can be encrypted via a shared
 // encryption wrapper and supplied to perform just-in-time registration. The
 // public key contained in this bundle must match that within
-// FetchNodeCredentialsInfo, as must the nonce. Forgeries by other users with
+// FetchNodeCredentialsInfo, as must the nonce or registration challenge. Forgeries by other users with
 // access to the wrapper are prevented due to the signature on the
 // FetchNodeCredentialsRequest including this; replays are prevented because the
 // returned credentials are still encrypted to the derived shared key.
@@ -202,6 +202,7 @@ message WrappingRegistrationFlowInfo {
   bytes certificate_public_key_pkix = 2 [json_name="certificate_public_key_pkix"];
 
   bytes nonce = 20;
+  RegistrationChallenge registration_challenge = 30 [json_name="registration_challenge"];;
 
   google.protobuf.Struct application_specific_params = 50;
 }