Bug 1874322: add bcrypt as a supported hashing method for htpasswd passwords #186
Conversation
openshift-ci-robot
added
bugzilla/severity-low
|
@stlaz: This pull request references Bugzilla bug 1874322, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
needs-rebase
stlaz
force-pushed
the
bcrypt_hashes
branch
from
7d87b8e to
75cc639
Compare
openshift-ci-robot
removed
the
needs-rebase
|
/refresh |
|
/retest |
|
/bugzilla refresh |
|
@stlaz: This pull request references Bugzilla bug 1874322, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
|
@stlaz: This pull request references Bugzilla bug 1874322, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/valid-bug
|
/retest |
|
|
||
| bcryptPrefix := realPassword[:4] | ||
| if bcryptPrefix == "$2a$" || bcryptPrefix == "$2x$" || bcryptPrefix == "$2y$" { | ||
| return bcrypt.CompareHashAndPassword([]byte(realPassword), []byte(password)) == nil |
There was a problem hiding this comment.
you intentionally pass along the $2whatever? Also, where did that format come from?
There was a problem hiding this comment.
this is what upstream of this project did before it went archived, the format comes from https://svn.apache.org/viewvc/apr/apr/trunk/crypto/crypt_blowfish.c?view=markup#l580
There was a problem hiding this comment.
suggestion: maybe let's add a comment to this external reference
|
I don't think this will actually resolve the bug since the bug is about the possibility to store passwords in sha1, not the requirement to do so. Also, why do we attempt to verify passwords in this proxy at all? That doesn't make much sense to me. |
|
@deads2k we can't remove the sha1-logins as that would break everyone who is using those, unfortunately. This is not exactly password verification for the oauth-users, it is a htpasswd config that allows to bypass the oauth-login. I think it might be thought about as kind of a service-account idp for the proxy. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci-robot
removed
the
lifecycle/stale
|
agreed with @stlaz, this just adds the bcrypt capability, while not removing sha1 |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: s-urbaniak, stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-component |
|
@stlaz: All pull requests linked via external trackers have merged: Bugzilla bug 1874322 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build golang-github-openshift-oauth-proxy-container-v4.8.0-202311261141.p0.g3fc0d89.assembly.stream for distgit golang-github-openshift-oauth-proxy. |
The PR is based on bitly@008ffae with the expection of not including the
$2b$prefix based on https://svn.apache.org/viewvc/apr/apr/trunk/crypto/crypt_blowfish.c?view=markup#l580