-
Notifications
You must be signed in to change notification settings - Fork 579
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
Secure by default sessions #2200
Open
stigtsp
wants to merge
1
commit into
mojolicious:main
Choose a base branch
from
stigtsp:secure-by-default-sessions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+278
−26
Open
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 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 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 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 |
---|---|---|
|
@@ -9,7 +9,8 @@ use Mojo::Home; | |
use Mojo::Loader; | ||
use Mojo::Log; | ||
use Mojo::Server; | ||
use Mojo::Util; | ||
use Mojo::Util qw(urandom_urlsafe); | ||
use Mojo::File qw(path); | ||
use Mojo::UserAgent; | ||
use Mojolicious::Commands; | ||
use Mojolicious::Controller; | ||
|
@@ -41,14 +42,31 @@ has plugins => sub { Mojolicious::Plugins->new }; | |
has preload_namespaces => sub { [] }; | ||
has renderer => sub { Mojolicious::Renderer->new }; | ||
has routes => sub { Mojolicious::Routes->new }; | ||
has secrets_file => sub { $ENV{MOJO_SECRETS_FILE} || shift->home->rel_file('mojo.secrets') }; | ||
has secrets => sub { | ||
my $self = shift; | ||
my $file = $self->secrets_file; | ||
|
||
# Warn developers about insecure default | ||
$self->log->trace('Your secret passphrase needs to be changed (see FAQ for more)'); | ||
if (-f $file) { | ||
|
||
# Default to moniker | ||
return [$self->moniker]; | ||
# Read secrets and filter out those who are less than 22 characters long | ||
# (~128 bits), as they are not likely to be sufficiently strong. | ||
my @secrets = grep { length $_ >= 22 } split /\n/, path($file)->slurp; | ||
|
||
die qq{"Your secrets_file "$file" does not contain any acceptable secret (of 22 chars or more)} unless @secrets; | ||
|
||
return [@secrets]; | ||
} | ||
|
||
# If no secrets file exists, generate one and attempt to write it back to | ||
# secrets_file, taking care that the file is only readable by the current | ||
# user. | ||
my $secret = urandom_urlsafe; | ||
path($file)->touch->chmod(0600)->spew($secret); | ||
|
||
$self->log->trace(qq{Your secret passphrase has been set to strong random value and stored in "$file"}); | ||
|
||
return [$secret]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I think this is generally a good and necessary step, this will invalidate session cookies in currently insecure apps so that will need to at minimum be noted in the changelog (which is not the responsibility of this PR). |
||
}; | ||
has sessions => sub { Mojolicious::Sessions->new }; | ||
has static => sub { Mojolicious::Static->new }; | ||
|
@@ -496,11 +514,13 @@ endpoints for your application. | |
my $secrets = $app->secrets; | ||
$app = $app->secrets([$bytes]); | ||
Secret passphrases used for signed cookies and the like, defaults to the L</"moniker"> of this application, which is | ||
not very secure, so you should change it!!! As long as you are using the insecure default there will be debug messages | ||
in the log file reminding you to change your passphrase. Only the first passphrase is used to create new signatures, | ||
but all of them for verification. So you can increase security without invalidating all your existing signed cookies by | ||
rotating passphrases, just add new ones to the front and remove old ones from the back. | ||
Secret passphrases used for signed cookies and the like, defaults to 256 bits of data from your systems secure | ||
random number generator and is stored in the file mojo.secrets in your MOJO_HOME directory. You can override | ||
the location of this file by setting MOJO_SECRETS_FILE in your environment. | ||
Only the first passphrase is used to create new signatures, but all of them for verification. So you can | ||
increase security without invalidating all your existing signed cookies by rotating passphrases, just add new | ||
ones to the front and remove old ones from the back. | ||
# Rotate passphrases | ||
$app->secrets(['new_passw0rd', 'old_passw0rd', 'very_old_passw0rd']); | ||
|
This file contains 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 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,82 @@ | ||
package Mojolicious::Command::Author::generate::secret; | ||
stigtsp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
use Mojo::Base 'Mojolicious::Command'; | ||
use Mojo::File qw(path); | ||
use Mojo::Util qw(urandom_urlsafe); | ||
|
||
has description => 'Generate secret'; | ||
has usage => sub { shift->extract_usage }; | ||
|
||
sub run { | ||
my ($self, $secret_file) = (shift, shift); | ||
|
||
$secret_file //= $self->app->secrets_file; | ||
|
||
my $token = urandom_urlsafe(); | ||
|
||
print "Writing secret to $secret_file\n"; | ||
|
||
path($secret_file)->touch->chmod(0600)->spew($token); | ||
} | ||
|
||
1; | ||
|
||
=encoding utf8 | ||
=head1 NAME | ||
Mojolicious::Command::Author::generate::secret - Secret generator command | ||
=head1 SYNOPSIS | ||
Usage: APPLICATION generate secret [PATH] | ||
mojo generate secret | ||
mojo generate secret /path/to/secret | ||
Options: | ||
-h, --help Show this summary of available options | ||
=head1 DESCRIPTION | ||
L<Mojolicious::Command::Author::generate::secret> generates a secret token for protecting session cookies | ||
This is a core command, that means it is always enabled and its code a good example for learning to build new commands, | ||
you're welcome to fork it. | ||
See L<Mojolicious::Commands/"COMMANDS"> for a list of commands that are available by default. | ||
=head1 ATTRIBUTES | ||
L<Mojolicious::Command::Author::generate::secret> inherits all attributes from L<Mojolicious::Command> and implements | ||
the following new ones. | ||
=head2 description | ||
my $description = $app->description; | ||
$app = $app->description('Foo'); | ||
Short description of this command, used for the command list. | ||
=head2 usage | ||
my $usage = $app->usage; | ||
$app = $app->usage('Foo'); | ||
Usage information for this command, used for the help screen. | ||
=head1 METHODS | ||
L<Mojolicious::Command::Author::generate::secret> inherits all methods from L<Mojolicious::Command> and implements | ||
the following new ones. | ||
=head2 run | ||
$app->run(@ARGV); | ||
Run this command. | ||
=head1 SEE ALSO | ||
L<Mojolicious>, L<Mojolicious::Guides>, L<https://mojolicious.org>. | ||
=cut |
This file contains 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 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 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 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 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 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 @@ | ||
NeverGonnaGiveYouUpNeverGonnaLetYouDown |
This file contains 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,3 @@ | ||
NeverGonnaMakeYouCryNeverGonnaSayGoodbye | ||
skip-me | ||
NeverGonnaTellALieAndHurtYou |
This file contains 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,16 @@ | ||
use Mojo::Base -strict; | ||
|
||
use Mojo::File qw(tempdir path); | ||
use Test::Mojo; | ||
use Test::More; | ||
use Mojolicious::Lite; | ||
|
||
|
||
my $tmpdir = tempdir; | ||
my $file = $tmpdir->child("mojo.secrets"); | ||
$ENV{MOJO_SECRETS_FILE} = $file; | ||
|
||
like app->secrets->[0], qr/^[-A-Za-z0-9_]{43}$/, 'secret was generated, and matches expected urandom_urlsafe format'; | ||
is app->secrets->[0], $file->slurp, 'secret stored at $ENV{MOJO_SECRETS_FILE} is the same as app->secrets->[0]'; | ||
|
||
done_testing(); |
Oops, something went wrong.
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.
Should there be a warning message if a candidate secret is skipped? Right now, they're discarded silently.
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.
Yes - or maybe even a fatal error?
Btw, I'm not sure how useful more than one key in
app->secrets
is. With a strong secret, wouldn't rotation only makes sense if it is compromised/leaked/etc? And then you wouldn't want to continue trusting the old key imho.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.
It is useful so that instances using the session key can be upgraded to use the new secret over even a short period of time without immediately invalidating everything.
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.
Also, regular secret rotation is good practice even when not found to be compromised.
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 think only the first secret (the one used to set new session cookies) should be subject to the length restriction, since you might still want to restore sessions cookies that were set with crappy secrets so you can re-issue them with new stronger key...