feat: smime signed notification emails
What does this MR do?
Add a new optional ActionMailer interceptor that signs all outgoing emails with SMIME
- I checked this configuration working against a dummy gmail account with actual SMTP delivery in gdk; Gmail web and native Outlook on macOS were ok with the SMIME signing.
- We have identified that
letter_opener_web
does not play nice with the signed emails, so they cannot currently be checked in development. Opened https://github.com/fgrehm/letter_opener_web/issues/96 to track this (check the attached screenshots in there). - Per guidelines, this MR will require another Omnibus MR to add the new configuration options (I'd also say they need to be added to gdk? though this is not documented in https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/settings/gitlab.yml.md)
Corresponding MR on omnibus for the new smime configuration params: omnibus-gitlab!3514 (merged)
Issue for adding new config parameters to k8s chart: https://gitlab.com/charts/gitlab/issues/1533
Issue for supporting eliptic curve keys: https://gitlab.com/gitlab-org/gitlab-ce/issues/66439
The development of this MR is sponsored by siemens (/cc @bufferoverflow).
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 n/a -
Separation of EE specific content n/a
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 n/a
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 1st contribution Community contribution labels
@smcgivern, would this be in ~Plan's camp since this seems to be primarily related to email notifications?
@jeremy yes. I don't think that matters too much for the code review, but probably it does for the PM side of things, especially as there's no issue for this MR.
@ebrinkman could you take a look please?
added Plan [DEPRECATED] groupproject management labels
added 138 commits
-
9b98b892...92d536a0 - 137 commits from branch
gitlab-org:master
- fce2e6ed - feat: SMIME signed notification emails
-
9b98b892...92d536a0 - 137 commits from branch
marked the checklist item Changelog entry as completed
marked the checklist item Documentation created/updated or follow-up review issue created as completed
- Resolved by James Lopez
@jameslopez I'm mentioning you following the gitlab contributing docs and also by geo proximity :-)
I'm working on the rspec for this MR and I'm faced with the need for an actual PKI infra for tests. I'm considering the following options:
- A dummy self-signed cert that is included as part of the MR and used as fixture, similar to mr_smime
- (same as above) but maybe this dummy cert should be setup as part of the gdk configuration step automatically
- An actual cert provided by Gitlab (the company) for testing purposes
Another open question is how to handle the expiration of such certs.
Suggestions on how you're doing this in other parts of the code? Thanks!
added 1 commit
- 6f8fa3ff - refactor certificate handling in preparation for spec testing
- Resolved by Diego Louzán
- Resolved by James Lopez
@dlouzan thanks! Left some comments. This is not something I'm very familiar with, so I'm not sure how useful they are!
assigned to @dlouzan
@dlouzan we may also want to wait until Product evaluates this, just in case this is found not to be useful for some reason (as there is no issue for it). cc @ebrinkman
@jameslopez Thanks, your suggestion about the kubernetes code was really helpful; I'll add some extra tests, and hopefully this will be checked soon.
added 1 commit
- 8ed2ad3c - refactor sign logic into own class and improve specs
added 1 commit
- 81895fcb - refactor sign logic into own class and improve specs
@ebrinkman Any news? Thanks!
@jameslopez Is there any way we can follow this up or raise its awareness? I'm sure there will be changes required and I'd prefer to do that while the code is fresh on my mind :-)
This is labelled as ~"group::team planning" so I'm gonna ping @gweaver here to see if this gets some attention.
As there is no associated issue so this was never planned, it may not be a priority to look at, so I apologise in advance!
Edited by James Lopezmarked the checklist item Code review guidelines as completed
marked the checklist item Tested in all supported browsers N/A as completed
marked the checklist item Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. as completed
marked the checklist item Merge request performance guidelines as completed
marked the checklist item Style guides as completed
marked the checklist item Database guides n/a as completed
marked the checklist item Separation of EE specific content n/a as completed
- Resolved by James Lopez
@dlouzan thanks for the work on this! It looks like there are some relevant open issues for improving S/MIME verification in several places (Email: https://gitlab.com/gitlab-org/gitlab-ee/issues/5816 • Commits: https://gitlab.com/gitlab-org/gitlab-ce/issues/63912 ). This is a great security improvement from a product standpoint.
@jameslopez @smcgivern I know this wasn't on our radar, but given a lot of work has been put into it, what can we do to support getting this over the line?
changed milestone to %12.2
assigned to @jameslopez and unassigned @dlouzan
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
Thanks @dlouzan - left some comments and suggestions :)
assigned to @dlouzan and unassigned @jameslopez
- Resolved by James Lopez
- Resolved by Diego Louzán
@jameslopez I have addressed all your comments, and left some of my own, please review.
Thanks @dlouzan! - left some more comments.
- Resolved by James Lopez
@jameslopez Back to you :-) Additionally I have a pending question in the issue description:
Per guidelines, this MR will require another Omnibus MR to add the new configuration options (I'd also say they need to be added to gdk? though this is not documented in https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/settings/gitlab.yml.md)
Does this also need a gdk MR, similar to Omnibus?
- Resolved by Diego Louzán
Thanks @dlouzan - this looks good! Thanks for all the effort here!
@smcgivern wanna give this a final look? Cheers!
assigned to @smcgivern and unassigned @dlouzan
added devopsplan label
- Resolved by Diego Louzán
- Corresponding MR on omnibus for the new smime configuration params
I'm just creating a thread for this so we don't forget about it.
- Resolved by Sean McGivern
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Russell Dickenson
- Resolved by Diego Louzán
- Resolved by Sean McGivern
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
Thanks @dlouzan! I have some questions for you, and some for my colleagues.
assigned to @dlouzan and unassigned @smcgivern
- Resolved by Diego Louzán
@jameslopez @smcgivern I'd also like to ask about another thing I documented in the description of the MR:
We have identified that
letter_opener_web
does not play nice with the signed emails, so they cannot currently be checked in development. Opened https://github.com/fgrehm/letter_opener_web/issues/96 to track this (check the attached screenshots in there).Should anybody else familiar with that be involved here?
added 1760 commits
-
50f8cc3b...d8966abd - 1759 commits from branch
gitlab-org:master
- f72bb428 - feat: SMIME signed notification emails
-
50f8cc3b...d8966abd - 1759 commits from branch
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Diego Louzán
- Resolved by Sean McGivern
@smcgivern I have addressed all topics, the remaining wait for yours or your team's feedback, back to you
mentioned in commit siemens/omnibus-gitlab@4afb1e3c
mentioned in commit siemens/omnibus-gitlab@21ebb6b4
mentioned in commit siemens/omnibus-gitlab@df1e7bd4
mentioned in merge request omnibus-gitlab!3514 (merged)
mentioned in commit siemens/omnibus-gitlab@79888b87
assigned to @estrike
- Resolved by Diego Louzán
mentioned in issue #66439 (moved)
@smcgivern All threads resolved and got an approval, I think we are good to go
- Resolved by Diego Louzán
changed milestone to %12.3
assigned to @smcgivern and unassigned @dlouzan and @estrike
added release post item label
mentioned in commit 842b4d4a
mentioned in merge request !32146 (merged)
mentioned in issue gitlab-development-kit#602 (closed)
mentioned in merge request gitlab-com/www-gitlab-com!30219 (merged)
mentioned in issue gitlab#5060
mentioned in merge request gitlab!18021 (merged)
mentioned in merge request gitlab!29352 (merged)
mentioned in merge request gitlab!76233 (merged)
mentioned in issue gitlab#5816