Skip to content

Gogs service password handling improvements#25116

Merged
7c6f434c merged 6 commits intoNixOS:masterfrom
rvl:gogs
May 1, 2017
Merged

Gogs service password handling improvements#25116
7c6f434c merged 6 commits intoNixOS:masterfrom
rvl:gogs

Conversation

@rvl
Copy link
Copy Markdown
Contributor

@rvl rvl commented Apr 22, 2017

Motivation for this change

Part of #24288.

Tested on nixos-unstable branch, https://git.rodney.id.au/rodney/nixpkgs

/cc @schneefux @basvandijk

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • [/] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

rvl added 2 commits April 22, 2017 17:07
I was getting a secret key like this:

  [security]
  SECRET_KEY = 7X

Use coreutils base64 instead to get the full 256 bits of randomness.
@mention-bot
Copy link
Copy Markdown

@rvl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schneefux and @jgertm to be potential reviewers.

Directory which contains the config file /var/lib/gogs already
has mode 700 but users are liable to change these things.
KEY=$(head -c 16 /dev/urandom | tr -dc A-Za-z0-9)
sed -i "s,#secretkey#,$KEY,g" ${cfg.stateDir}/custom/conf/app.ini
cp -f ${configFile} ${runConfig}
KEY=$(head -c 16 /dev/urandom | base64)
Copy link
Copy Markdown
Member

@Mic92 Mic92 Apr 23, 2017

Choose a reason for hiding this comment

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

Is it good practice to regenerate this secret key on every start? I could imagine that this will invalidate all session cookies?
cc @unknwon

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an oversight by me, the key should only be written once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same thing, so I tried restarting gogs but didn't lose my login session. Let us knwo @unknwon

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a fix to generate the key first time, but am now slightly concerned that the SECRET_KEY setting isn't being used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, the SECRET_KEY setting isn't used for session cookies. There seem to be two session cookies and two different ways of generating them. One encrypts with the user's password, the other I'm not sure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Mic92: I think we are OK from the NixOS module side. I have changed it so that SECRET_KEY is only generated on the first start. After checking the Gogs code I have found that SECRET_KEY is used for 2-factor auth and CSRF token generation. Session cookies are generated from user password + random.

@7c6f434c 7c6f434c merged commit 938fbf6 into NixOS:master May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants