Add 'id' prefix to generated SAML IDs (#2046)

Though the SAML spec does not specify what the contents of the ID must
be, the Azure IdP implementation prohibits it beginning with a number.
We follow their suggestion to prefix with 'id'.

See https://docs.microsoft.com/en-us/azure/active-directory/develop/single-sign-on-saml-protocol.

Fixes #2044.

server/sso/validate.go

@@ -169,13 +169,17 @@ func (v *validator) validateAssertionSignature(elt *etree.Element) error {
 }
 
 const (
+	idPrefix   = "id"
 	idSize     = 16
 	idAlphabet = `1234567890abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ`
 )
 
-// There isn't anything in the SAML spec that tells us what is valid inside an ID
-// other than expecting that it has to be unique and valid XML. ADFS blows up on '=' in the ID,
-// so we are using an alphabet that we know works.
+// There isn't anything in the SAML spec that tells us what is valid inside an
+// ID other than expecting that it has to be unique and valid XML. ADFS blows
+// up on '=' in the ID, so we are using an alphabet that we know works.
+//
+// Azure IdP requires that the ID begin with a character so we use the constant
+// prefix.
 func generateSAMLValidID() (string, error) {
 	randomBytes := make([]byte, idSize)
 	_, err := rand.Read(randomBytes)
@@ -185,5 +189,5 @@ func generateSAMLValidID() (string, error) {
 	for i := 0; i < idSize; i++ {
 		randomBytes[i] = idAlphabet[randomBytes[i]%byte(len(idAlphabet))]
 	}
-	return string(randomBytes), nil
+	return idPrefix + string(randomBytes), nil
 }

server/sso/validate_test.go

@@ -187,3 +187,10 @@ func TestIDGenerator(t *testing.T) {
 		idTable[id] = struct{}{}
 	}
 }
+
+func TestIDPrefix(t *testing.T) {
+	// Ensure ID comes with the appropriate prefix
+	id, err := generateSAMLValidID()
+	require.Nil(t, err)
+	assert.Equal(t, idPrefix, id[:2])
+}