Sanitize SSH pubkeys (and Support multiple) when sync from LDAP

[strk]

In our setup we allow multiple SSH keys to be entered in the LDAP database, for each user.
When Gitea synchonizes those keys it does so by creating a wrong configuration, including newlines (which separate the SSH keys) in the authorized_key, basically allowing direct ssh access.

[strk]

The above behaviour was tested with version 1.12.3

[zeripath]

Gitea asks LDAP to provide it with the SSHPublicKey attributes and collects them as a []string

gitea/modules/auth/ldap/ldap.go

Line 336 in 729f0f5

var sshPublicKey []string

gitea/modules/auth/ldap/ldap.go

Lines 387 to 389 in 729f0f5

	if isAttributeSSHPublicKeySet {
	sshPublicKey = sr.Entries[0].GetAttributeValues(ls.AttributeSSHPublicKey)
	}

It then passes that array to and individually adds them as separate public keys:

gitea/models/user.go

Lines 1588 to 1609 in 729f0f5

	func addLdapSSHPublicKeys(usr *User, s *LoginSource, sshPublicKeys []string) bool {
	var sshKeysNeedUpdate bool
	for _, sshKey := range sshPublicKeys {
	_, _, _, _, err := ssh.ParseAuthorizedKey([]byte(sshKey))
	if err == nil {
	sshKeyName := fmt.Sprintf("%s-%s", s.Name, sshKey[0:40])
	if _, err := AddPublicKey(usr.ID, sshKeyName, sshKey, s.ID); err != nil {
	if IsErrKeyAlreadyExist(err) {
	log.Trace("addLdapSSHPublicKeys[%s]: LDAP Public SSH Key %s already exists for user", s.Name, usr.Name)
	} else {
	log.Error("addLdapSSHPublicKeys[%s]: Error adding LDAP Public SSH Key for user %s: %v", s.Name, usr.Name, err)
	}
	} else {
	log.Trace("addLdapSSHPublicKeys[%s]: Added LDAP Public SSH Key for user %s", s.Name, usr.Name)
	sshKeysNeedUpdate = true
	}
	} else {
	log.Warn("addLdapSSHPublicKeys[%s]: Skipping invalid LDAP Public SSH Key for user %s: %v", s.Name, usr.Name, sshKey)
	}
	}
	return sshKeysNeedUpdate
	}

The issue here must be that instead of having separate attributes for each public key you must just have a single attribute containing all of the public keys. Is that really the correct way to store these?

[strk]

On Mon, Dec 14, 2020 at 06:55:29AM -0800, zeripath wrote: The issue here must be that instead of having separate attributes for each public key you must just have a single attribute containing all of the public keys. Is that really the correct way to store these?

I don't know if it's correct or not, possibly not, but still I think Gitea should be made more tolerant to these kind of errors, checking that the each element of the retrived array is a valid single-line ssh key, to avoid assumptions which may open security holes.

[zeripath]

so it's strange because at first glance Gitea is attempting to assert that thing is a valid public key

gitea/models/user.go

Line 1591 in 729f0f5

_, _, _, _, err := ssh.ParseAuthorizedKey([]byte(sshKey))

but the problem is that line is an abuse of that function. That function is there parse out the valid ssh-key lines one by one.

I think this should just be fixed to just parse the string reading in all of the keys one by one

[zeripath]

We could go further than the attached PR - we use this function in a similar way in another place

gitea/models/ssh_key.go

Lines 110 to 207 in c3fc190

	// parseKeyString parses any key string in OpenSSH or SSH2 format to clean OpenSSH string (RFC4253).
	func parseKeyString(content string) (string, error) {
	// remove whitespace at start and end
	content = strings.TrimSpace(content)
	var keyType, keyContent, keyComment string
	if strings.HasPrefix(content, ssh2keyStart) {
	// Parse SSH2 file format.
	// Transform all legal line endings to a single "\n".
	content = strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(content)
	lines := strings.Split(content, "\n")
	continuationLine := false
	for _, line := range lines {
	// Skip lines that:
	// 1) are a continuation of the previous line,
	// 2) contain ":" as that are comment lines
	// 3) contain "-" as that are begin and end tags
	if continuationLine || strings.ContainsAny(line, ":-") {
	continuationLine = strings.HasSuffix(line, "\\")
	} else {
	keyContent += line
	}
	}
	t, err := extractTypeFromBase64Key(keyContent)
	if err != nil {
	return "", fmt.Errorf("extractTypeFromBase64Key: %v", err)
	}
	keyType = t
	} else {
	if strings.Contains(content, "-----BEGIN") {
	// Convert PEM Keys to OpenSSH format
	// Transform all legal line endings to a single "\n".
	content = strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(content)
	block, _ := pem.Decode([]byte(content))
	if block == nil {
	return "", fmt.Errorf("failed to parse PEM block containing the public key")
	}
	pub, err := x509.ParsePKIXPublicKey(block.Bytes)
	if err != nil {
	var pk rsa.PublicKey
	_, err2 := asn1.Unmarshal(block.Bytes, &pk)
	if err2 != nil {
	return "", fmt.Errorf("failed to parse DER encoded public key as either PKIX or PEM RSA Key: %v %v", err, err2)
	}
	pub = &pk
	}
	sshKey, err := ssh.NewPublicKey(pub)
	if err != nil {
	return "", fmt.Errorf("unable to convert to ssh public key: %v", err)
	}
	content = string(ssh.MarshalAuthorizedKey(sshKey))
	}
	// Parse OpenSSH format.
	// Remove all newlines
	content = strings.NewReplacer("\r\n", "", "\n", "").Replace(content)
	parts := strings.SplitN(content, " ", 3)
	switch len(parts) {
	case 0:
	return "", errors.New("empty key")
	case 1:
	keyContent = parts[0]
	case 2:
	keyType = parts[0]
	keyContent = parts[1]
	default:
	keyType = parts[0]
	keyContent = parts[1]
	keyComment = parts[2]
	}
	// If keyType is not given, extract it from content. If given, validate it.
	t, err := extractTypeFromBase64Key(keyContent)
	if err != nil {
	return "", fmt.Errorf("extractTypeFromBase64Key: %v", err)
	}
	if len(keyType) == 0 {
	keyType = t
	} else if keyType != t {
	return "", fmt.Errorf("key type and content does not match: %s - %s", keyType, t)
	}
	}
	// Finally we need to check whether we can actually read the proposed key:
	_, _, _, _, err := ssh.ParseAuthorizedKey([]byte(keyType + " " + keyContent + " " + keyComment))
	if err != nil {
	return "", fmt.Errorf("invalid ssh public key: %v", err)
	}
	return keyType + " " + keyContent + " " + keyComment, nil
	}

This has a different set of acceptable SSH keys.

Likely these two functions should be made more similar.

[strk]

I'm reopening this as I just had a chance to test 1.14.0+dev-423-gb4f8da533e and it still doesn't seem to support multiple keys in a single attribute. Nothing is printed in logs about the reason why a single key is extracted when there are two in the ldap database. Both keys are in the same element of the attribute array (array of a single attribute)

[zeripath]

@strk you're gonna have to give us an example of a value that doesn't work.

[strk]

This is what I'm getting out of LDAP:

ssh-rsa FIRSTKEY strk@cin\01503d
        ssh-rsa SECONDKEY strk@cdb