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

Check the cipher flags to see if the cipher supports aead #7223

Merged

Conversation

danielwestendorf
Copy link
Contributor

Supports checking if a cipher supports aead, with an authenticated? helper method.

@danielwestendorf
Copy link
Contributor Author

Need is explained here.

@danielwestendorf danielwestendorf force-pushed the feature/authenticated-cipher branch from e2e1a30 to 24a7f28 Compare December 27, 2018 20:02
@danielwestendorf
Copy link
Contributor Author

The darwin CI suite seems to be failing because aes-128-gcm isn't an available cipher, however, when run locally on my macOS system (with Crystal installed via homebrew) it is available and the spec passes.

@ysbaddaden
Copy link
Contributor

CI seems to run on macOS sierra, which probably has an old OpenSSL version (maybe 0.9.8zh).

Do you know which OpenSSL versions support aes-128-gcm?

@danielwestendorf danielwestendorf force-pushed the feature/authenticated-cipher branch 3 times, most recently from b94b377 to 8e781ed Compare December 27, 2018 21:29
@danielwestendorf
Copy link
Contributor Author

@ysbaddaden I think you're on to something there. I'm trying different macOS/OSX versions to see what the oldest version this change would require is, as I can't seem to find any definitive information about when aes-128-gcm was added to OpenSSL.

@danielwestendorf danielwestendorf force-pushed the feature/authenticated-cipher branch 2 times, most recently from 327f76b to 6407f2f Compare December 27, 2018 22:15
Supports checking if a cipher supports aead, with an `authenticated?` helper method.
@danielwestendorf danielwestendorf force-pushed the feature/authenticated-cipher branch 2 times, most recently from 267bb88 to b853500 Compare December 28, 2018 14:25
@danielwestendorf
Copy link
Contributor Author

danielwestendorf commented Dec 28, 2018

It looks like GCM ciphers aren't supported on macOS v10.12 and lower. When I updated the CI config to xcode: "9.3.0" the specs passed 💯.

For now, I've reverted to xcode: "9.0" as it currently is, and simply rescued the ArgumentError in the spec. This is definitely a code smell, but probably better than bumping the CI macOS version we test against. Once it makes sense for Crystal's CI macOS to v10.13+ the rescue can be removed.

.circleci/config.yml Outdated Show resolved Hide resolved
macOS v10.12 and lower don't support GCM ciphers, so the `authetnicated?` method will never return true.

CI currently targets macOS v10.12, this rescue should be removed when CI targets >= 10.13.
@danielwestendorf danielwestendorf force-pushed the feature/authenticated-cipher branch from b853500 to 2a7e2ed Compare December 28, 2018 15:11
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

I'd like @bcardiff's thoughts on rescuing ArgumentError in the specs before merge.

@ysbaddaden ysbaddaden added this to the 0.27.1 milestone Dec 28, 2018
begin
cipher = OpenSSL::Cipher.new("aes-128-gcm")
cipher.authenticated?.should eq(true)
rescue ArgumentError
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a check that its actually an expected ArgumentError by verifying the error message. It should be re-raised if it doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could definitely do that, however if the ArgumentError is originating from anything other than the cipher not being available, it'd raise on the next invocation (line 58/59).

@RX14 RX14 merged commit 929003b into crystal-lang:master Dec 29, 2018
@bcardiff
Copy link
Member

I'm late to the party. The changes are fine. At most I would've try to only accept the exception as a success if the error message is certain, or wrap the whole block of the it in a feature check if.

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

Successfully merging this pull request may close these issues.

6 participants