Support "ecdsa-sk" and "ed25519-sk" SSH keys
What does this MR do and why?
Related to #213259 (closed).
This MR provides support "ecdsa-sk" and "ed25519-sk" SSH keys.
In !77374 (merged), !77403 (merged), !77996 (merged), !77424 (merged), and !78532 (merged) we have done the work that facilitates support "ecdsa-sk" and "ed25519-sk" SSH keys.
By adding support "ecdsa-sk" and "ed25519-sk" SSH keys, we provide a new, more secure, and easy-to-use way to strongly authenticate with Git while preventing unintended and potentially malicious access. For instance, if a user's private key file on their computer is stolen, it would be useless without the user's security key.
Read:
- OpenSSH 8.2 release notes: https://www.openssh.com/releasenotes.html#8.2
- OpenSSH's support for U2F/FIDO security keys: https://github.com/openssh/openssh-portable/blob/8a0848cdd3b25c049332cd56034186b7853ae754/PROTOCOL.u2f
- https://cloud.google.com/compute/docs/tutorials/ssh-with-sk
- https://www.yubico.com/blog/github-now-supports-ssh-security-keys/
- https://github.blog/2021-05-10-security-keys-supported-ssh-git-operations/
Changelog: added
Screenshots or screen recordings
Demo: Using "ecdsa-sk" and "ed25519-sk" SSH keys - https://www.youtube.com/watch?v=DtmZEVguN7g
Database changes
AddEcdsaSkAndEd25519SkKeyRestrictionsToApplicationSettings
migration is reversible:
bogdanvlviv@lenovo:~/gitlab-development-kit/gitlab$ bin/rails db:migrate
== 20220128093756 AddEcdsaSkAndEd25519SkKeyRestrictionsToApplicationSettings: migrating
-- add_column(:application_settings, :ecdsa_sk_key_restriction, :integer, {:default=>0, :null=>false})
-> 0.0020s
-- add_column(:application_settings, :ed25519_sk_key_restriction, :integer, {:default=>0, :null=>false})
-> 0.0016s
== 20220128093756 AddEcdsaSkAndEd25519SkKeyRestrictionsToApplicationSettings: migrated (0.0037s)
bogdanvlviv@lenovo:~/gitlab-development-kit/gitlab$ bin/rails db:rollback
== 20220128093756 AddEcdsaSkAndEd25519SkKeyRestrictionsToApplicationSettings: reverting
-- remove_column(:application_settings, :ed25519_sk_key_restriction, :integer, {:default=>0, :null=>false})
-> 0.0018s
-- remove_column(:application_settings, :ecdsa_sk_key_restriction, :integer, {:default=>0, :null=>false})
-> 0.0013s
== 20220128093756 AddEcdsaSkAndEd25519SkKeyRestrictionsToApplicationSettings: reverted (0.0044s)
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
assigned to @bdenkovych
changed milestone to %14.8
- A deleted user
added backend label
1 Warning 782a634c: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines. 1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/api/settings.md
doc/push_rules/push_rules.md
doc/security/ssh_keys_restrictions.md
doc/security/two_factor_authentication.md
doc/ssh/index.md
doc/user/admin_area/settings/visibility_and_access_controls.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Reuben Pereira ( @rpereira2
) (UTC+5.5, 3.5 hours ahead of@bdenkovych
)Alex Pooley ( @alexpooley
) (UTC+8, 6 hours ahead of@bdenkovych
)database Alexandru Croitor ( @acroitor
) (UTC+2, same timezone as@bdenkovych
)Tiger Watson ( @tigerwnz
) (UTC+11, 9 hours ahead of@bdenkovych
)~migration No reviewer available No maintainer available test Quality for spec/features/*
Valerie Burton ( @vburton
) (UTC-6, 8 hours behind@bdenkovych
)Maintainer review is optional for test Quality for spec/features/*
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded typefeature label
added featureaddition label
added 1 commit
- fbfcaee7 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- 2376eb20 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
- Resolved by Alex Pooley
@bdenkovych, please can you answer the question: Should this have a feature flag? to help with code review for the Access group.This nudge was added by this triage-ops policy.
Setting label(s) ~"Category:Authentication and Authorization" devopsmanage sectiondev based on ~"group::authentication and authorization".
added devopsmanage sectiondev + 1 deleted label
added 711 commits
-
2376eb20...d3fc18ff - 710 commits from branch
master
- 689df0e4 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
-
2376eb20...d3fc18ff - 710 commits from branch
- Resolved by Alex Pooley
added 1 commit
- b6781ff6 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- f2f86ccf - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- 30d6d2bf - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- ca079182 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
- Resolved by Alex Pooley
- A deleted user
added database databasereview pending labels
added 1 commit
- 99b8c7bd - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
- Resolved by Alex Pooley
added 1 commit
- d59e117b - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- 6a5ae3b0 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
- A deleted user
added documentation label
Allure report
allure-report-publisher
generated test report for 782a634c!review-qa-smoke:
test report
review-qa-reliable: test reportmentioned in issue gitlab-org/quality/testcases#2445 (closed)
mentioned in merge request golang-crypto!1 (merged)
added 1 commit
- e62a44a1 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- 88bdc3b5 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- c6e4c411 - Draft: Support "ecdsa-sk" and "ed25519-sk" SSH keys
added 1 commit
- 97da2dc8 - Support "ecdsa-sk" and "ed25519-sk" SSH keys
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- Resolved by Alex Pooley
removed databasereview pending label
added databasereview pending label
- Resolved by Bogdan Denkovych
@eread Could you please review documentation?
requested review from @eread