Skip to content

Conversation

@herwinw
Copy link
Member

@herwinw herwinw commented Oct 10, 2023

A lot of error messages depend on the used OpenSSL version, so I had to be a bit lenient with the error matching.

@eregon
Copy link
Member

eregon commented Oct 10, 2023

length = 16
hash = "sha1"
key = OpenSSL::KDF.pbkdf2_hmac(pass, salt: salt, iterations: iterations, length: length, hash: hash)
key.should == "!\x99+\xF0^\xD0\x8BM\x158\xC4\xAC\x9C\xF1\xF0\xE0".b
Copy link
Member

@andrykonchin andrykonchin Oct 11, 2023

Choose a reason for hiding this comment

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

suggestion: as far as salt/iterations/length... are passed as keyword arguments - using local variables seems excessive to me. It doesn't improve readability of a test and don't add context (as far as meaning is already expressed with keyword argument names) but makes test case longer:

    pass = "secret"
    key = OpenSSL::KDF.pbkdf2_hmac(pass, salt: "\x00".b * 16, iterations: 20_000, length: 16, hash: "sha1")
    
    key.should == "!\x99+\xF0^\xD0\x8BM\x158\xC4\xAC\x9C\xF1\xF0\xE0".b

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't exactly proud of this repetitive code either.

Since it's mostly just a bunch of default with one argument changed or removed, I was thinking maybe it's better to create a set of defaults, and the tests should explicitly alter these values. Something like this:

before :each do
  @pass = "secret"
  @defaults = { salt: "\x00".b * 16, iterations: 20_000, ... }
end

it "works with 10_000 iterations do"
  key = OpenSSL::KDF.pbkdf2_hmac(@pass, **@defaults, iterations: 10_000)
  key.should == SOME_VALUE
end

This reduces the length of the tests, removes a lot of duplication, and makes it a bit more clear which parameter we're testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed an update with the defaults hash, I would think this is much clearer and concise.

@herwinw herwinw force-pushed the openssl_kdf_pbkdf2_hmac branch 2 times, most recently from 654eb5c to 461f532 Compare October 26, 2023 17:07
@herwinw
Copy link
Member Author

herwinw commented Oct 26, 2023

Today I Learned: the value of OpenSSL::VERSION is the version of the gem, and not the version of OpenSSL. The Windows issues have been fixed now.

@andrykonchin
Copy link
Member

Could you please squash commits?

@herwinw herwinw force-pushed the openssl_kdf_pbkdf2_hmac branch from 7cbe6f5 to 2516f65 Compare October 27, 2023 14:29
@herwinw
Copy link
Member Author

herwinw commented Oct 27, 2023

Could you please squash commits?

They're squashed into a single commit.

@andrykonchin
Copy link
Member

Thank you!

@andrykonchin andrykonchin merged commit 0c8e2d1 into ruby:master Oct 27, 2023
@herwinw herwinw deleted the openssl_kdf_pbkdf2_hmac branch October 27, 2023 17:51
@eregon
Copy link
Member

eregon commented Nov 27, 2023

@herwinw FYI: 92f6df8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants