Skip to content
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

cmd/syncthing: Accept pre-hashed password in config POST (fixes #4458) #4466

Closed
wants to merge 3 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Oct 26, 2017

It must be a bcrypt hash.

Tested manually to change password to and from stuff and pasting in a hash in the password field in the GUI.

If your actual password looks like $2a$10$(50 characters or more) you're going to have a bad day. I'd wager that doesn't happen very often.

@@ -790,7 +791,8 @@ func (s *apiService) postSystemConfig(w http.ResponseWriter, r *http.Request) {
}

if to.GUI.Password != s.cfg.GUI().Password {
if to.GUI.Password != "" {
bcryptExpr := regexp.MustCompile(`^\$2[aby]\$\d+\$.{50,}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this a global variable to compile only once?

@DrSchnagels
Copy link

When I backup the config on my Android phone, then I can see the GUI password.
I remember that @Nutomic hardcoded a password in one of the releases. ( it is 0.9.13. )

   <gui enabled="true" tls="false" debugging="false">
        <address>127.0.0.1:8384</address>
        <user>syncthing</user>
        <password>$2a$04$H***************************************</password>
        <apikey>********************</apikey>
        <theme>dark</theme>
    <tls>true</tls></gui>

If your actual password looks like $2a$10$(50 characters or more) you're going to have a bad day. I'd wager that doesn't happen very often.

Will I have a bad day too?

@calmh
Copy link
Member Author

calmh commented Oct 27, 2017

No. You're fine.

@AudriusButkevicius
Copy link
Member

Apart from @imsodin's comment, LGTM.

@calmh
Copy link
Member Author

calmh commented Nov 6, 2017

@st-review merge these relations into a consolidated set of 3nf relations

@st-review
Copy link

@calmh: Build status is pending. I'll wait until it goes green and then merge!

@st-review
Copy link

👌 Merged as 941c9f1. Thanks, @calmh!

st-review pushed a commit that referenced this pull request Nov 6, 2017
It must be a bcrypt hash.

GitHub-Pull-Request: #4466
@st-review st-review closed this Nov 6, 2017
@calmh calmh deleted the fix-4458 branch November 7, 2017 07:27
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 7, 2018
@syncthing syncthing locked and limited conversation to collaborators Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants