-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved security support #197
Improved security support #197
Conversation
This proposal seem to cover all the points brought up in #185 using standardized ways, which is great. What I'm skeptical to is the inclusion of the public key in the message. This addition hints at just verify the signature with the provided public key not a previous registered public key that has a known origin. It is not a problem per se as it is up to the developer to do the right thing but I have a hard time to see the benefit of it. |
Thank you for the input! The (optional) inclusion of the public key was actually added at the suggestion of the security experts I consulted over this. I personally don't have any strong opinion on this either way, but I can see the utility of it. Given that it's optional, anyone who finds it problematic or bad practice to include the public key can simply choose not to. |
The primary use I see for the included signing key is to indicate this was the key this was signed with as one could assume there are multiple different sources who could be signing the data, with different keys, and we would not necessarily have to assume a strong PKI with renewal handling etc is in place for all applications.
After this, you will know the further messages are signed by "the same" actor as before by comparing the keys, you just cannot be sure "who" the actor is as no PKI or pre-sharing has created this linkage. This also limits the window of man-in-the-middle attacks, if the listener is only willing to accept keys for sources it has not seen before or for a limited period of time. In other words, I absolutely agree with David's concern that when you get a first message and the key to validate it with, you gain no security because an attacker can change the key as they modify the message. But when you get the SECOND message from the same source, you can gain security happiness from the fact that the first message gave you a key and you cleverly kept it so this is "the same sender" even though it could be a persistent long-con attacker too. (UIs can be built on whether you want to accept anonymous keys, whether you want to allow changing them later etc, but if the key sharing slot does not appear in the protocol, the key distribution will have to be provided by a completely separate system.) Disclaimer: It shall be noted that I'm commenting this on a reasonably abstract PKI point of view, not as an expert on JSON signature formats. I'm also not a huge expert on Eiffel, but boldly commenting based on a briefing on the subject from Daniel. :) |
Changed documentation, schema and example of EiffelArtifactPublishedEvent. This is intended to enable review and discussion of the solution, before applying to all event types.
4750922
to
76fcbc4
Compare
This PR has been rebased to master (following #195) and event type documentation, schemas and examples have been updated. Please review and comment! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a typo in the documentation of all events I think this looks good syntactically and the documentation is probably understandable. But I am by no means a security expert, so I doubt my experience when it comes to reviewing this change from a security perspective. I cannot say if there are any security holes in the way the properties are defined in this proposal. Could you ask one of the 'security experts' you say you have consulted for this task to provide a statement on the level of security introduced with this PR?
__Description:__ The name of the sequence. There MUST not be two identical __meta.security.sequenceProtection.sequenceName__ values in the same event. | ||
|
||
##### meta.security.sequenceProtection.sequenceName | ||
__Type:__ Integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. The key should be 'position'. The same typo seems to have been copied to several other events as well.
Thanks! Yes, the above conversation in the PR thread are from the main stakeholder requesting these improvements and from said security expert, respectively. As long as they are happy with the conceptual soundness of the PR, and you find the syntax and documentation acceptable, I think we're good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I accept that.
Just an additional minor thing. You've added a new example event for EiffelTestCaseTriggeredEvent but the 2.0.0 version of that event is not linked from the documentation. I think it would be good to do it the same way as is done in the documentation of EiffelTestExecutionRecipeCollectionCreatedEvent. Or maybe we should skip those specific example event links from the documentation and instead link to the folder containing all example events for the corresponding event?
Good point. Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a security expert, and I can't really comment about any aspect around that topic. But from a protocol definition and documentation view it looks good.
Thanks! |
Applicable Issues
#185.
Description of the Change
This change follows the proposal of issue #185: meta.security has been updated to offer improved author identification and sequence protection (guarding e.g. against replay attacks or data loss). Furthermore, the signing mechanism has been updated and tightened, influenced by JSON Web Signatures and Canonical JSON Form.
NOTE: This is not a complete pull request: all event types need to be updated and version stepped. To avoid unnecessary rework, I'm posting this now to invite comments and criticism. Once we agree on the changes I will apply them to all event types.
Alternate Designs
What is proposed here is not "vanilla JWS". This is something of a compromise in order to keep Eiffel principle and overall document structure, while providing comparable functionality.
Benefits
Improved integrity protection, better alignment with existing standards and improved user convenience.
Possible Drawbacks
I don't think there are any real drawbacks compared to the previous solution. Please correct me if I'm wrong :)
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: Daniel Ståhl [email protected]