feat: x509 signed commits using openssl
What does this MR do?
This MR provides basic support for x509 signed commits as proposed within #24512 (closed) and #29782 (closed).
Main difference in comparision to gpg:
- trust anchor is the certificate authority not a verified key
- no specific key uploaded by the user
- verification status could be one of the following
- verified: email within x509 certificate equals committer email and ca is trusted
- unverified: email within x509 certificate does not match committer email or ca is not trusted or signature invalid
- verification can be done using plain OpenSSL functionality or via gpgsm
- my initial approach was using gpgsm, however the formats and concepts of x509 and gpg keys differ heavily and requires a lot of workarounds when using gpgsm, beside of the additional complexity at the code level gpp does require a lot of local configuration. This will make it error prone and reduce the operatability significantly
- the openssl native approach is much simpler from an implementation, maintenance and operations point of view
Approach:
-
add a new Gitlab::SignedCommit
class containing has_signature?, signature_type and signature_data extract function -
add a new Gitlab::X509::Commit
class similar toGitlab::Gpg::Commit
-
verify x509 signature using OpenSSL and the default certs ruby -ropenssl -e 'puts OpenSSL::X509::DEFAULT_CERT_FILE'
-
add a worker to update the x509 signatures, similar to this one https://gitlab.com/gitlab-org/gitlab/blob/master/app/workers/create_gpg_signature_worker.rb (moved to #122159 (closed)) -
create a follow-up issue for CRL handling (#122159 (closed)) -
Create follow-up for tag handling (#122157 (closed)), currently blocked by gitaly#2120 (closed)
Further reading:
- Git upstream support for x509 signed commits from my team mate Henning Schild, see https://public-inbox.org/git/20180706011834.GD7697@genre.crustytoothpaste.net/
- GitHub Announcement: https://github.blog/changelog/2018-09-10-smime-signature-verification/
- GitHub Help: https://help.github.com/en/articles/about-commit-signature-verification#smime-commit-signature-verification
The MR we made to introduce GPG signed commits within GitLab:
Samples of SMIME signed commits:
-
https://gitlab.com/gitlab-org/gitlab-test/commits/smime-signed-commits , requires http://www.siemens.com/pki/ZZZZZZA1.crt as pem file(
openssl x509 -inform DER -outform PEM -in ZZZZZZA1.crt -out ZZZZZZA1.pem
) within cert file located atruby -ropenssl -e 'puts OpenSSL::X509::DEFAULT_CERT_FILE'
- signed tag: https://gitlab.com/gitlab-org/gitlab-test/-/tags/v1.1.1
Screenshots
Database
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation created/updated or follow-up review issue created -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Performance and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
added Community contribution label
added customer label
added devopscreate groupsource code labels
cc/ @nick.thomas
added 2 commits
added 172 commits
-
886f4be0...17fc6231 - 167 commits from branch
gitlab-org:master
- 63447004 - feat: x509 signed commits using openssl
- 73126a93 - chore: use system certificate store
- 6deb66f7 - refactor: use dedicated haml templates for x509
- a84ddbe7 - chore: update db/schema.rb
- df020131 - refactor: remove unused code section from x509 badge
Toggle commit list-
886f4be0...17fc6231 - 167 commits from branch
@kerrizor do you mind taking a look at this one as a traintainer? Once you're happy with the code, pick a random maintainer for it :)
mentioned in issue gitlab-development-kit#627 (closed)
- Resolved by Roger Meier
added 1 commit
- 6cf3bbaf - fix(commit-model): load signature_data only if project is defined
added 1 commit
- 5eb4dfa2 - fix(commit-model): load signature_data only if project is defined
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Kerri Miller
- Resolved by Roger Meier
Thanks for submitting the MR!
I skimmed over the HAML when I realized my knowledge of it is sadly lacking; we should probably get a frontend reviewer to take a look at that.
One thing that I didn't see included here were any spec changes or additions; we'll want to add tests for the frontend additions as well as the new
X509Signatures
class.
added 1 commit
- 0333f5a8 - refactor: change several sections based on reviewer input
mentioned in merge request gitlab-test!40 (closed)
added 1 commit
- 2cb92ed9 - docs(repository): add x509 signed commits doc
added 1 commit
- 122e9c4f - refactor: add user_id and cert issuer, display cert subject/issuer
added 1 commit
- 4fce6ed9 - refactor: strong_memoize signature_type within commit model
added 1 commit
- c2d8fdea - fix: ensure safe navigation within signature_text and signed_text
added 1 commit
- 9bc0823c - docs: add changelog file for x509 signed commits
marked the checklist item Changelog entry as completed
added 1 commit
- cb43488a - refactor: do not show fingerprint, limit to CN,OU,O attributes
- Resolved by Roger Meier
@henning-schild What do you think, shall keyUsage attribute contain "Digital Signature" for a valid signature ? see also https://github.com/github/smimesign/issues/61
added 1353 commits
-
cb43488a...1bea9238 - 1336 commits from branch
gitlab-org:master
- 7fa3b8c8 - feat: x509 signed commits using openssl
- 52910dd6 - chore: use system certificate store
- e7ffeda7 - refactor: use dedicated haml templates for x509
- a7a9f6ac - chore: update db/schema.rb
- 199543d1 - refactor: remove unused code section from x509 badge
- 4a37db1c - fix(commit-model): load signature_data only if project is defined
- e91ea436 - refactor: change several sections based on reviewer input
- 9369c8d3 - docs(repository): add x509 signed commits doc
- 0973dee5 - refactor: add user_id and cert issuer, display cert subject/issuer
- 4bd59490 - refactor: strong_memoize signature_type within commit model
- e3df076c - fix: ensure safe navigation within signature_text and signed_text
- 6478b296 - docs: add changelog file for x509 signed commits
- 1afc3390 - docs: add link text to get rid of bare url
- de56fd53 - refactor: do not show fingerprint, limit to CN,OU,O attributes
- 65a61ab6 - fix: issues from rebase
- b7fa3da7 - fix: x509 documentation link text
- 5c070896 - feat: add x509 related text to gitlab.pot
Toggle commit list-
cb43488a...1bea9238 - 1336 commits from branch
added 1 commit
- 583af232 - refactor: remove keyUsage==Digital Signature enforcement
- Resolved by Roger Meier
- Resolved by Roger Meier
added 1 commit
- 681a2123 - fix: add second variant of pgp signature header
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
added documentation label
added 1 commit
- 532b6a6f - fix: add return for :none case within signature_type
added 1 commit
- 7147e190 - refactor: simplify access to signature_text and signed_text
added 1 commit
- 19f2132f - refactor: avoid & operator within initializer
added 1 commit
- ef6868b2 - docs: use global instead of local, minor changes
added 1 commit
- 4d3c00f6 - refactor: introduce signed commit parent class
added 1 commit
- 6e2b35e0 - refactor(smime): do not include OpenSSL to avoid name conflicts
added 1 commit
- 62563127 - refactor(smime): do not include OpenSSL to avoid name conflicts within specs
- Resolved by Roger Meier
added 1 commit
- bfb56cb3 - fix: smime specs fail because of new Gitlab::X509 module scoping
added 1 commit
- c96c462f - fix: only load signature if commit.project is defined
added database databasereview pending labels
added 8 commits
- b2b52852 - feat: show user for unverified x509 signatures
- 653511b3 - refactor: x509 class to be inline with gpg
- fbb036a3 - fix: corner case for signature handling
- 1457af47 - test: add initial x509 signed commit spec
- 88534d67 - refactor: cleanup x509_signature
- 75708ac2 - test: load trusted cert from x509 helper
- 8a600f91 - feat: update x509 signature worker
- b04a3e58 - test: add update x509 signature worker spec
Toggle commit list- Resolved by Roger Meier
@kerrizor I'm very sorry for the delay! However, I was able to work a bit on this and would appriciate if you could take a look at the current state. I've addeed specs and an update worker including specs now. Beside of this I did several refactorings.
added 1361 commits
-
b04a3e58...80bd6f78 - 1318 commits from branch
gitlab-org:master
- 3cb89232 - feat: x509 signed commits using openssl
- ad84091d - chore: use system certificate store
- e6f81cf0 - refactor: use dedicated haml templates for x509
- ad07afb7 - chore: update db/schema.rb
- 9e40e85d - refactor: remove unused code section from x509 badge
- 21bc6bf2 - fix(commit-model): load signature_data only if project is defined
- 87b7d630 - refactor: change several sections based on reviewer input
- 811846f5 - docs(repository): add x509 signed commits doc
- 1dcbc9dd - refactor: add user_id and cert issuer, display cert subject/issuer
- 1231a008 - refactor: strong_memoize signature_type within commit model
- 4b6d0dbb - fix: ensure safe navigation within signature_text and signed_text
- 33316d13 - docs: add changelog file for x509 signed commits
- 894b7d73 - docs: add link text to get rid of bare url
- c1c9894e - refactor: do not show fingerprint, limit to CN,OU,O attributes
- 64716140 - fix: issues from rebase
- 531bccc6 - fix: x509 documentation link text
- 041924f8 - feat: add x509 related text to gitlab.pot
- 117fdea0 - refactor: remove keyUsage==Digital Signature enforcement
- 93c43b5e - fix: add second variant of pgp signature header
- 6fecff4c - chore: move rubocop to individual line
- b2aba906 - style: add extra new line for better readability
- 77e9c32e - fix: add return for :none case within signature_type
- 992a249e - refactor: simplify access to signature_text and signed_text
- a2cb9386 - fix: use break instead of return within signature_type
- 133d5fa6 - refactor: use belongs_to :user instead of user method
- fecfe781 - refactor: avoid & operator within initializer
- 531a0ee5 - fix: change from 5.1 to 5.2
- fc23c92e - refactor: use next instead of break within strong_memoize
- a97a1040 - refactor: use belongs_to instead of a commit function
- 5c28e312 - docs: use global instead of local, minor changes
- 8c185bf7 - refactor: introduce signed commit parent class
- 5866a7ca - refactor(smime): do not include OpenSSL to avoid name conflicts
- b3fdfefc - refactor(smime): do not include OpenSSL to avoid name conflicts within specs
- 3f4aff32 - fix: smime specs fail because of new Gitlab::X509 module scoping
- 5fca5e54 - fix: only load signature if commit.project is defined
- 03dc5afe - feat: show user for unverified x509 signatures
- c4270fb7 - refactor: x509 class to be inline with gpg
- 29844f38 - fix: corner case for signature handling
- 1ec1e859 - test: add initial x509 signed commit spec
- dbf4878d - refactor: cleanup x509_signature
- 2bbdb8fa - test: load trusted cert from x509 helper
- 80c25f87 - feat: update x509 signature worker
- 455bedf8 - test: add update x509 signature worker spec
Toggle commit list-
b04a3e58...80bd6f78 - 1318 commits from branch
@kerrizor just rebased as well
- Resolved by Roger Meier
- Resolved by Roger Meier
- Resolved by Roger Meier
added 29 commits
- 1007e4ee - fix: issues from rebase
- 97f66642 - fix: x509 documentation link text
- de5013e4 - feat: add x509 related text to gitlab.pot
- 60e0dbc7 - refactor: remove keyUsage==Digital Signature enforcement
- b2c05b13 - fix: add second variant of pgp signature header
- 73f6fd4f - chore: move rubocop to individual line
- 11d31295 - style: add extra new line for better readability
- 2cf3fc43 - fix: add return for :none case within signature_type
- 01d270f0 - refactor: simplify access to signature_text and signed_text
- 791d21cc - fix: use break instead of return within signature_type
- 1fd16f62 - refactor: use belongs_to :user instead of user method
- a9929746 - refactor: avoid & operator within initializer
- 2763ac71 - fix: change from 5.1 to 5.2
- 029db131 - refactor: use next instead of break within strong_memoize
- f48f287e - refactor: use belongs_to instead of a commit function
- c90a65b1 - docs: use global instead of local, minor changes
- 24c955c4 - refactor: introduce signed commit parent class
- 59b81394 - refactor(smime): do not include OpenSSL to avoid name conflicts
- 655af0b8 - refactor(smime): do not include OpenSSL to avoid name conflicts within specs
- 6ac1fb34 - fix: smime specs fail because of new Gitlab::X509 module scoping
- 58fb21e2 - fix: only load signature if commit.project is defined
- 7af0cc4d - feat: show user for unverified x509 signatures
- b5f7b8e3 - refactor: x509 class to be inline with gpg
- fa8459cf - fix: corner case for signature handling
- d6cc9a42 - test: add initial x509 signed commit spec
- b552c1e1 - refactor: cleanup x509_signature
- bf256691 - test: load trusted cert from x509 helper
- 5b775484 - feat: update x509 signature worker
- 2f512985 - test: add update x509 signature worker spec
Toggle commit list- Resolved by Toon Claes