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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent the update of the portus user #1896

Merged
merged 3 commits into from Jul 24, 2018

Conversation

Projects
None yet
3 participants
@mssola
Contributor

mssola commented Jul 23, 2018

Summary

This pull request introduces two commits that prevents someone from updating the portus hidden user. This user is not to be used externally (i.e. it's not an explicit admin), but it's used internally for requests performed against the registry. That being said, if an attacker has access to the email being set for this hidden user, then it may be able to reset the password and use it externally too. This is now mitigated:

  1. The update on the portus hidden user is no longer allowed (at the model level).
  2. An initializer has been introduced that updates the password of this hidden user depending on the password secret. This is useful when the administrator thinks that this secret has been compromised and wants to update it: simply set a new secret and restart portus.
  3. The "I forgot my password" workflow has been disabled for this hidden user.

Thanks @kiorky for noticing this bug 馃憦

Fixes #1878

To be done

  • Port to the v2.3 branch (#1897).
  • Update the documentation on the second point (#1898).

mssola added some commits Jul 23, 2018

user: do not allow the update of the portus user
The only exception is on the password, that may change depending on
whether the given secret changed or not. For this case, Portus will
always update the password of the portus user on start.

Fixes #1878

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>
passwords: don't allow the portus user to reset
This commit disallows someone knowing the portus email from attempting
to reset the password through the "I forgot my password" feature. This
attack wouldn't work anyways because of the change introduced in
16a7b11, where updating the portus user
is explicitly disabled (except when changing the secret for the
password).

See #1878

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>
@vitoravelino

This comment has been minimized.

Show comment
Hide comment
@vitoravelino

vitoravelino Jul 23, 2018

Member

LGTM.

One question: should we also prevent any update on the api level too?

Member

vitoravelino commented Jul 23, 2018

LGTM.

One question: should we also prevent any update on the api level too?

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 23, 2018

Contributor

One question: should we also prevent any update on the api level too?

This is done at the model level, so regardless on whether it was attempted from a controller or the api, the same validation error will be raised. In the case of the API, if they try to do this, they will get a 422 (general validation error), which is fine.

Contributor

mssola commented Jul 23, 2018

One question: should we also prevent any update on the api level too?

This is done at the model level, so regardless on whether it was attempted from a controller or the api, the same validation error will be raised. In the case of the API, if they try to do this, they will get a 422 (general validation error), which is fine.

@kiorky

This comment has been minimized.

Show comment
Hide comment
@kiorky

kiorky Jul 23, 2018

Contributor

We should be able to reset the password via the API, in case for exemple of resetting manually via irb, this kind of code was what i used actually:

    portusctl exec rails r '
     User.where(id=1).first.unlock_access!
     User.where(id=1).first.update!(
       email: "{{cops_portus_admin_mail}}",
       password: Rails.application.secrets.portus_password)'

BUT, i saw also the introduction of the initializer in the PR.
Can a ruby mong confirm me that this will also update the password on any configuration change + portus restart and not only on the first database init.

In this case, then, so far so good, as we have a way to reset that so particular user password and the rest of the workflow protect access to that user 馃憤 .

Contributor

kiorky commented Jul 23, 2018

We should be able to reset the password via the API, in case for exemple of resetting manually via irb, this kind of code was what i used actually:

    portusctl exec rails r '
     User.where(id=1).first.unlock_access!
     User.where(id=1).first.update!(
       email: "{{cops_portus_admin_mail}}",
       password: Rails.application.secrets.portus_password)'

BUT, i saw also the introduction of the initializer in the PR.
Can a ruby mong confirm me that this will also update the password on any configuration change + portus restart and not only on the first database init.

In this case, then, so far so good, as we have a way to reset that so particular user password and the rest of the workflow protect access to that user 馃憤 .

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 23, 2018

Contributor

We should be able to reset the password via the API, in case for exemple of resetting manually via irb, this kind of code was what i used actually:

I disagree: if this password is only set as a secret, it should be kept this way. If we introduce multiple ways of providing the same thing, it will be confusing and lead to possible future bugs. So, let's keep this simple: it can only be provided as a secret.

BUT, i saw also the introduction of the initializer in the PR.
Can a ruby mong confirm me that this will also update the password on any configuration change + portus restart and not only on the first database init.

Exactly. So in Rails initializers are always executed when starting the application. In this case, it will set the password as defined by the secret on start. So, if you want to reset the modification you just did with IRB, simply restart Portus and it will set back your defined secret 馃憤

Contributor

mssola commented Jul 23, 2018

We should be able to reset the password via the API, in case for exemple of resetting manually via irb, this kind of code was what i used actually:

I disagree: if this password is only set as a secret, it should be kept this way. If we introduce multiple ways of providing the same thing, it will be confusing and lead to possible future bugs. So, let's keep this simple: it can only be provided as a secret.

BUT, i saw also the introduction of the initializer in the PR.
Can a ruby mong confirm me that this will also update the password on any configuration change + portus restart and not only on the first database init.

Exactly. So in Rails initializers are always executed when starting the application. In this case, it will set the password as defined by the secret on start. So, if you want to reset the modification you just did with IRB, simply restart Portus and it will set back your defined secret 馃憤

user: skip validations when creating portus user
This is a fix for a regression introduced by the previous two commits.

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>

@mssola mssola merged commit 5f4caa2 into SUSE:master Jul 24, 2018

2 checks passed

DCO All commits have a DCO sign-off from the author
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mssola mssola deleted the mssola:portus-email branch Jul 24, 2018

mssola added a commit that referenced this pull request Jul 24, 2018

docs: added note on how the update portus password
See #1896

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>
@kiorky

This comment has been minimized.

Show comment
Hide comment
@kiorky

kiorky Jul 24, 2018

Contributor

@mssola i confirm that it is the intended behavior, the irb thing was at that moment the only way that was available, and the intializer approach is way nicer 馃憤 .

Contributor

kiorky commented Jul 24, 2018

@mssola i confirm that it is the intended behavior, the irb thing was at that moment the only way that was available, and the intializer approach is way nicer 馃憤 .

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 24, 2018

Contributor

The 2.3 and latest docker images have already been updated with these fixes. I've tested them locally and everything looks alright.

Contributor

mssola commented Jul 24, 2018

The 2.3 and latest docker images have already been updated with these fixes. I've tested them locally and everything looks alright.

mssola added a commit to mssola/Portus that referenced this pull request Jul 27, 2018

Fixed regression on registries not being created
This commit fixes a regression introduced in the pull request SUSE#1896 in
which registries were no longer creatable. This happened because of a
validation error on `portus_user_validation` produced by adding a team
to the portus hidden user.

Fixes SUSE#1909

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>

mssola added a commit to mssola/Portus that referenced this pull request Jul 27, 2018

Fixed regression on registries not being created
This commit fixes a regression introduced in the pull request SUSE#1896 in
which registries were no longer creatable. This happened because of a
validation error on `portus_user_validation` produced by adding a team
to the portus hidden user.

Fixes SUSE#1909

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>

mssola added a commit that referenced this pull request Jul 27, 2018

Fixed regression on registries not being created
This commit fixes a regression introduced in the pull request #1896 in
which registries were no longer creatable. This happened because of a
validation error on `portus_user_validation` produced by adding a team
to the portus hidden user.

Fixes #1909

Signed-off-by: Miquel Sabat茅 Sol脿 <msabate@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment