-
Notifications
You must be signed in to change notification settings - Fork 242
auth: disable sha1 by default on non-windows #413
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
Merged
+14
−1
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| //go:build !windows | ||
| // +build !windows | ||
|
|
||
| package dbus | ||
|
|
||
| func getDefaultAuthMethods(user string) []Auth { | ||
| return []Auth{AuthExternal(user)} | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package dbus | ||
|
|
||
| func getDefaultAuthMethods(user string) []Auth { | ||
| return []Auth{AuthCookieSha1(user, getHomeDir())} | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 fine with changing the default behavior to not use this mechanism on non-Windows platforms, but why make the
AuthCookieSha1definition completely Windows-only? Users can still explicitly specific authentication mechanisms, so this change would remove the possibility to use this mechanism (and strictly speaking, be a breaking change since this removes an exported type). Even thoughEXTERNALis pretty much always used when it works (i.e. on Unix), maybe some users have cases where even on Unix,EXTERNALcan't be used but SHA1 can.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.
Because all usage of SHA1 must be deprecated. I am also trying to build applications that embed go-dbus without ability to use SHA1 anymore.
I do not believe any Linux distribution has working and configured SHA1 Auth that is secure. Nor can it be used securely.
Is there any pre-existing evidence of any SHA1 usage on Linux? I posit, if there was, it would have been upgraded to sha256 by now.
If such pre-existing deployments exist I can add an opt-in build tag for this. However it is insecure, and dbus spec needs upgrades to sha256.
Cc @martinpitt any advice here? Or who else to ask?
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 understand. Generally I try to stick to what the official spec says, and there it is still mentioned as the "recommended authentication mechanism for platforms and configurations where EXTERNAL cannot be used."
I advise you to then open an issue with the spec itself: https://gitlab.freedesktop.org/dbus/dbus/-/issues
I can't judge how much / whether this is actually used - my gut feeling is that this is pretty rare, but I have been surprised before. We can merge this and see if any bug reports come in; but as long as the spec still mentions this mechanism, I'm prepared to revert this PR if this actually causes issues for users.
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.
https://gitlab.freedesktop.org/dbus/dbus/-/issues/564