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

feat(security): Implement Bcrypt for basic auth and deprecate SHA512 #233

Merged
merged 4 commits into from
Jan 9, 2022

Conversation

TwiN
Copy link
Owner

@TwiN TwiN commented Jan 9, 2022

I've re-written the code for this several times in the past before but always ended up not going all the way through with it because a hashed Bcrypt string has dollar signs in it, which caused issues with the config file due to environment variable support.

I finally decided to go through with it by forcing users to base64 encode the bcrypt hash.


The bottom line is, it was irresponsible of me to add support for SHA512 in the first place; it is a fast hash, and is not intended to be used as password.

I've re-written the code for this several times before but always ended up not going through with it because a hashed Bcrypt string has dollar signs in it, which caused issues with the config due to environment variable support.
I finally decided to go through with it by forcing users to base64 encode the bcrypt hash
@TwiN TwiN added the feature New feature or request label Jan 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2022

Codecov Report

Merging #233 (37af22f) into master (fc016bd) will increase coverage by 0.07%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   83.20%   83.27%   +0.07%     
==========================================
  Files          52       52              
  Lines        3293     3313      +20     
==========================================
+ Hits         2740     2759      +19     
- Misses        430      431       +1     
  Partials      123      123              
Impacted Files Coverage Δ
controller/handler/handler.go 86.11% <0.00%> (-2.47%) ⬇️
security/sha512.go 100.00% <ø> (ø)
security/config.go 73.77% <86.95%> (+2.65%) ⬆️
security/basic.go 100.00% <100.00%> (ø)
config/maintenance/maintenance.go 96.20% <0.00%> (+3.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc016bd...37af22f. Read the comment docs.

@TwiN TwiN merged commit c466542 into master Jan 9, 2022
@TwiN TwiN deleted the bcrypt branch January 9, 2022 00:59
@TwiN
Copy link
Owner Author

TwiN commented Jan 9, 2022

Released in v3.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants