FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bcrypt_ext: Add compatibility with libxcrypt by besser82 · Pull Request #164 · bcrypt-ruby/bcrypt-ruby · GitHub

  1. bcrypt-ruby
  2. bcrypt-ruby

Repository navigation

bcrypt_ext: Add compatibility with libxcrypt - #164

Merged
tenderlove merged 2 commits into
bcrypt-ruby:masterfrom
besser82:libxcrypt
May 11, 2018
ConversationCommits (2)ChecksFiles changed
Merged

bcrypt_ext: Add compatibility with libxcrypt#164
tenderlove merged 2 commits into
bcrypt-ruby:masterfrom
besser82:libxcrypt

Conversation

besser82 commented Feb 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

The return values of crypt() and crypt_gensalt() should be treated as real c-strings instead of binary data by the module.

besser82 commented Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

The failing tests are not releated to this change.

jdoss commented Apr 5, 2018

Copy link
Copy Markdown

Any ETA on getting this merged? Bcrypt on with ruby-2.5.0-90.fc28.x86_64 on Fedora 28 Beta when using https://github.com/codahale/bcrypt-ruby/blob/master/lib/bcrypt/password.rb#L46 puts a bunch of null bytes on the end of the output.

We pointed our Gemfile at https://github.com/besser82/bcrypt-ruby/tree/libxcrypt and it totally fixes the problem.

ai mentioned this pull request Apr 22, 2018
Closed
tenderlove reviewed Apr 24, 2018
View reviewed changes

tenderlove left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This PR looks good, but I'm curious which change was actually necessary to fix the bug. Seems like only the second change is necessary (and maybe we should just change it to rb_str_new2).

Comment thread ext/mri/bcrypt_ext.c Outdated
if(!salt) return Qnil;

str_salt = rb_str_new2(salt);
str_salt = rb_str_new(salt, strlen(salt));

tenderlove Apr 24, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this actually necessary to fix the bug? It seems like rb_str_new2 already does that. (rb_str_new2 is just an alias for rb_str_new_cstr)

besser82 commented Apr 25, 2018

Copy link
Copy Markdown
Contributor Author

I've updated the PR.

ojab commented May 1, 2018

Copy link
Copy Markdown

JFYI: Fedora-28 with libxcrypt is released.

frenkel mentioned this pull request May 2, 2018
Closed

duritong commented May 3, 2018

Copy link
Copy Markdown

Fedora 28 was release 2 days ago and at the moment you can't use bcrypt on it. Can we get that fixed? Thanks!

voxik commented May 3, 2018

Copy link
Copy Markdown

@duritong dnf install rubygem-bcrypt should do the job.

ojab commented May 3, 2018

Copy link
Copy Markdown

Unfortunately it's not a solution for rvm/rbenv/etc-based workflow.

Merge branch 'master' into libxcrypt
92f33ae 
* master:
  Use RBX 3
  Try updating Bundler too
  Test on more Rubies in CI; looser version definition
  Update RG and see if that fixes the build
  Update lockfile so newer Ruby works with JSON gem
tenderlove merged commit 5c2d2d3 into bcrypt-ruby:master May 11, 2018
tjschuck mentioned this pull request May 15, 2018
Merged

tjschuck commented May 15, 2018

Copy link
Copy Markdown
Collaborator

I have just pushed version 3.1.12.rc1 to RubyGems.org which should solve this problem.

Can someone please confirm that gem install bcrypt --prerelease now works on Fedora 28? If so, I'll push out the final version.

ojab commented May 15, 2018

Copy link
Copy Markdown

Works fine here (F28 w/ all updates, ruby-2.5.0p0 via rvm), thanks.

patchkord commented May 16, 2018

Copy link
Copy Markdown

Yes, now everything is ok on F28

besser82 deleted the libxcrypt branch May 17, 2018 16:37
dennmart mentioned this pull request May 4, 2019
Closed

miry commented Sep 21, 2019

Copy link
Copy Markdown

Thanks. It fixes issue with

Sorcery::CryptoProviders::BCrypt.encrypt "Welcome2019", "ffLLizQt4eF_Dpd5mwcc"

raw_hash contains a huge binary string (sometime to other variables). It looks like size was calculated wrong for Fedora 30.

mmccarthy15 mentioned this pull request Sep 30, 2020
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tenderlove tenderlove left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL