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

[strk]

The above string is what is printed upon login with this patch:

diff --git a/models/login_source.go b/models/login_source.go
index 1de24a9cf7..687eb8597b 100644
--- a/models/login_source.go
+++ b/models/login_source.go
@@ -458,11 +458,14 @@ func composeFullName(firstname, surname, username string) string {
 // and create a local user if success when enabled.
 func LoginViaLDAP(user *User, login, password string, source *LoginSource) (*User, error) {
        sr := source.Cfg.(*LDAPConfig).SearchEntry(login, password, source.Type == LoginDLDAP)
+
        if sr == nil {
                // User not in LDAP, do nothing
                return nil, ErrUserNotExist{0, login, 0}
        }
 
+       log.Warn("User: %s publicKey: %s", sr.Username, sr.SSHPublicKey)
+
        var isAttributeSSHPublicKeySet = len(strings.TrimSpace(source.LDAP().AttributeSSHPublicKey)) > 0
 
        // Update User admin flag if exist

I don't know what the \01503d string is about

[strk]

Interesting enough, google finds the \01503d string only in relation to Gitea: https://www.google.com/search?q=+%22\01503d%22

[strk]

Basically: GetAttributes returns a single attribute being a concatenation of 2 public keys. I guess Gitea is discarding anything after the first space (before the comment). I agree this is most probably a WRONG way to store public ssh keys in LDAP but would be nice if Gitea could report a problem of some kind (spurious comment? Unexpected newline ?)

[strk]

For the record: the probably WRONG format for ssh keys in our LDAP is discussed in https://trac.osgeo.org/osgeo/ticket/2542

[strk]

I finally fixed the way we store SSH keys in LDAP, and I confirm the support for multiple keys is working in master branch. And I confirm multiple keys support did NOT work in version 1.12.3 - Great work !

[strk]

For the record: I was wrong, 1.12.3 supports multiple keys just fine too, once they are correctly stored in LDAP

[zeripath]

I guess I just have no idea where that \01503d is coming from or what it should be interpreted as. Maybe it's some sort of escaped \r?

[strk]

For the record, I re-checked and once SSH keys are correctly stored in LDAP even the 1.12.3 version works fine

[strk]

I guess I just have no idea where that \01503d is coming from or what it should be interpreted as. Maybe it's some sort of escaped \r?

Probably, yes. or escaped \r\n. But the fact that Google only returns pages about Gitea makes me suspect escaping/decoding is somehow done in a wrong way ? I think LDAP used to return the field in base64 encoding, due to the presence of newlines. The decoding of base64 might have lost a carriage-return (the newline seemed to be there though)

[zeripath]

Hmm... now you say base64 - well that would explain the 03d which would probably represent an = and 015 represent a \r?

That would imply that the items are being split by \n (not \r\n) and then individually base64 encoded with the trailing \r forming part of the preceding block but it being an incomplete base64 triplet requiring a = for padding?

[strk]

Sounds very likely, yes. Now that I fixed the LDAP storage I indeed had to split by \r\n for it to work correctly and avoid the base64 encoding (not sure I understood the = part of this). In case this is helpful for handling such cases in LDAP this is all good info