-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Added Capability for Dotnetty to use Thumbprint for SSL #3629
Added Capability for Dotnetty to use Thumbprint for SSL #3629
Conversation
944ef11
to
9d0ba4f
Compare
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.
Left some comments on the tests
public void Secure_transport_should_be_possible_between_systems_using_thumbprint() | ||
{ | ||
// skip this test due to linux/mono certificate issues | ||
if (IsMono) return; |
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.
hmm. I think we would much rather have this test just run on mono/linux. And have us manually suppress the test in our CI env. Then let it silently complete as if everything is ok. @Aaronontheweb your thoughts?
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.
@Danthar unfortunately, skipping specific tests in some environments from within our build script is really tricky. We do this type of short-circuiting elsewhere inside the Akka.Remote test suite for the same reason.
While I agree it's not 100% clean, it's probably the best route for getting the job done.
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.
The code itself looks good to me, but the default HOCON needs some cleaning up before we merge this
|
||
thumpbrint = "" | ||
|
||
store-name = "My" |
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.
Leave this setting to empty, but also add a comment describing what it does
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.
Will Do. Will also do the same for store-location.
Speaking of which, should I clear the default of 'local-machine' for the store location? (At the very least, after consideration, 'current-user' is probably a better default if any.)
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.
@to11mtm going to ask a clarifying question here because I don't know the answer: why is current-user
a better default?
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 suppose it depends on one's environment.
current-user indicates that the certificate is in the user's (i.e. account the app is running under) store.
local-machine indicates a certificate that is in the machine's store. Local Machine can be cleaner if you've got different profiles that need to use the same cert, but conversely speaking means you need permissions to install a machine-level cert.
FWIW, 'local-machine' would be an analogue to the 'machine-key-set' option for a file cert, and 'current-user' would be an analogue to 'user-ket-set' (and, in most cases, 'default-key-set'.)
In that case, I think you're right. We should change it.
…Sent from my iPhone
On Oct 24, 2018, at 7:01 PM, to11mtm ***@***.***> wrote:
@to11mtm commented on this pull request.
In src/core/Akka.Remote/Configuration/Remote.conf:
> @@ -527,6 +527,17 @@ akka {
# Available flags include:
# default-key-set | exportable | machine-key-set | persist-key-set | user-key-set | user-protected
# flags = [ "default-key-set" ]
+
+ # To use a Thumbprint instead of a file, set this to true
+ # And specify a thumprint and it's storage location below
+ use-thumprint-over-file = false
+
+ thumpbrint = ""
+
+ store-name = "My"
I suppose it depends on one's environment.
current-user indicates that the certificate is in the user's (i.e. account the app is running under) store.
local-machine indicates a certificate that is in the machine's store. Local Machine can be cleaner if you've got different profiles that need to use the same cert, but conversely speaking means you need permissions to install a machine-level cert.
FWIW, 'local-machine' would be an analogue to the 'machine-key-set' option for a file cert, and 'current-user' would be an analogue to 'user-ket-set' (and, in most cases, 'default-key-set'.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
d0c98c9
to
78769ea
Compare
78769ea
to
b5e435a
Compare
Changes made+squashed. |
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.
Looks good to me
In certain environments, it is preferred to have certificates installed on the local machine by system administrators, and having applications use said certificates via thumbprint.
This pull request contains changes to allow thumbprints to be used by dotnetty,, via a new set of configuration options; backwards compatibility is maintained by this being 'opt in' behavior.