Validate the 'sync_ssh_keys' LDAP setting

Allowed values are: false and strings (an LDAP attribute name like sshkeys). The value true is NOT allowed.

I think we should detect this when parsing the LDAP settings during application startup. We just spent a lot of time finding this problem with a customer, based only on a cryptic error message during GitLab sign-in.

Something like:

if sync_ssh_keys && !sync_ssh_keys.is_a?(String)
  raise "Invalid sync_ssh_keys settings for LDAP server #{server}: #{sync_ssh_keys.inspect}"
end

cc @patricio

cc @JobV

Thanks for making this on .com @jacobvosmaer. 8.1 sounds good?

Milestone changed to 8.1

@JobV +1 for 8.1

Milestone changed to 8.3

Milestone changed to 8.4

Milestone changed to 8.6

Milestone changed to 8.7

Milestone changed to 8.8

Milestone changed to %8.9

Removed ~91797 ldap labels

Milestone changed to %8.10

@jacobvosmaer is this still relevant? How important is it?

@JobV this is still relevant. I think it is important to prevent lost time for administrators and our support team.

I don't know how often this has happened again since we created the issue. cc @dblessing

Yep, still an issue.

Added ldap label

Added ~360297 label

Added ~91797 label

Added ~111707 label

Removed ~111707 label

Milestone changed to %8.11

Added ~481018 label

Reassigned to @patricio

I think it's time we finally add this check. it shouldn't take more than a few hours to implement. I'll take care of it today.

FWIW, I'm not sure the value is anything other than sshPublicKey. You generally can't make up random attributes in LDAP.

@dblessing I just asked @northrup about it. The attribute where you store the SSH keys can be whatever you want, but we, as the consumer, could require that if you want to use SSH key synching with LDAP, the keys should be under sshPublicKey. That would brake existing installations that rely on it being somewhere else, though.

@patricio @northrup That's technically true although you can't just make it up on the fly. You have to actually create a schema with an objectclass that allows the attribute name and a string value.

If I'm trying to suggest anything it is that we do allow true as a possible value. If set to true, set the value to sshPublicKey and accept a string otherwise. I think the true value covers 99% of our users.

You're right - I should have looked because the LDAP servers that I've stood up I've extended... I just thought it was part of iNetOrgPerson. It's part of OpenSSH-LDAP extension and they have a registered OID for it (oid 1.3.6.1.4.1.24552.500.1.1.1.13) which is sshPublicKey. See https://github.com/AndriiGrytsenko/openssh-ldap-publickey

Then sshPublicKey sounds like a good default.

Thanks for the feedback @dblessing and @jacobvosmaer-gitlab. I'll add to the 1_settings.rb file that if sync_ssh_keys is set to true, we will set that to sshPublicKey in the backend.

mentioned in merge request !670 (merged)

Status changed to closed by merge request !670 (merged)

mentioned in commit 59ac8912

Mentioned in commit 59ac8912

mentioned in commit e67b6e82

mentioned in commit 467dbbb9

mentioned in commit a75c399c

mentioned in commit dc5cc692