change MD5 to SHA256

[nahumcohen]

The change of MD5 to SHA256 is required for the FIPS complaint requirements.
Should pay attention to

1. the CHANGELOG.md if the new version is required and correct
2. there is one check in the unit test 'understands both serializations' in jwt_spec that was remarked. There are no instruction how to generate it and reverse engineering helped with other tests but not with this one.

Outstanding work related to cyberark/conjur#1528

[shaharglazner]

@jvanderhoof
Do we know how to verify that this change does not break other packages?

[jvanderhoof]

@shaharglazner, @nahumcohen, let's make sure this change doesn't cause any issues in cyberark/conjur before we merge it. We can test by updating the Conjur Gemfile with the following:

gem 'slosilo', git: 'https://github.com/cyberark/slosilo.git', tag: 'sha256'

The above will install Slosilo from source. Full Bundler details are available here: https://bundler.io/guides/git.html.

After updating the gem reference in Conjur, we'll need to run bundle install and check-in the changes in a new branch (which won't be merged).

Assuming tests passed, we'll need to release a new version of Slosilo (the core team can handle that). The final step will be to bump the Slosilo version in cyberark/conjur.

[nahumcohen]

@jvanderhoof The cyberark/conjur consuming slosilo with the fix (https://jenkins.conjur.net/blue/organizations/jenkins/cyberark--conjur/detail/openssl-1.0.2/2/pipeline/408) is done on the openssl-1.0.2 branch (https://github.com/cyberark/conjur/tree/openssl-1.0.2).
Does it enough check? should it influence other repositories?

[alexkalish]

@nahumcohen: Could you please provide a more detailed description about this change and why it's being made? Answers to the following would be great:

• Briefly, how is MD5 currently used in the repo?
• Is this a breaking change for current downstream users?
• If it is breaking, are we incrementing the major version and providing some details about upgrading?

Thanks.

[nahumcohen]

@nahumcohen: Could you please provide a more detailed description about this change and why it's being made? Answers to the following would be great:

• Briefly, how is MD5 currently used in the repo?
• Is this a breaking change for current downstream users?
• If it is breaking, are we incrementing the major version and providing some details about upgrading?

Thanks.

• Briefly, how is MD5 currently used in the repo?

• It is used after login in authentication stage.
  slosilo is generating JWT token with MD5 encryption
  The slosilo stores the results in the DB in slosilo table in the fingerprint column.

• Is this a breaking change for current downstream users?

• It depends.
  Our code supplies a close loop. However, if client chose to rely/store the current JWT signature structure then it must be updated to the new structure in order to work.
  Regarding cyberark client only the OSS is affected only in (non-documented) upgrade process.

• If it is breaking, are we incrementing the major version and providing some details about upgrading?

• Guess the version should be incremented. Who should provide the new version?

[jvanderhoof]

I don't believe this is a breaking change as authentication tokens are short-lived (eight minutes). Connections would need to re-authenticate after the upgrade, but they need to do this regularly as part of basic operation.

[jvanderhoof]

@nahumcohen, can you please create a branch of cyberark/conjur with this version of Slosilo? This can be accomplished by replacing:

gem 'slosilo', '~> 2.1'

with:

gem 'slosilo', '~> 2.1', github: 'cyberark/slosilo', branch: 'sha256'

in the Gemfile, running bundle install and checking-in Gemfile and Gemfile.lock.

If tests pass on the branch of Conjur using this code, I'm comfortable releasing this change.

[alexkalish]

@jvanderhoof: Regarding your comment above about this change, are you saying that after upgrade, existing JWT tokens wouldn't be valid, but this is significantly mitigated by the fact that they are short lived? I assume that applies to both Conjur and DAP? 8 minutes isn't long, but it's also not short in machine time. Can anything else be done to mitigate this?

[uCatu]

I don't believe this is a breaking change as authentication tokens are short-lived (eight minutes). Connections would need to re-authenticate after the upgrade, but they need to do this regularly as part of basic operation.

@alexkalish @jvanderhoof
Hi - we also have the fingerprint of a tenant key in slosilo_keystore table that is not expired and not getting updated unless done by rake/manual task...

[nahumcohen]

I don't believe this is a breaking change as authentication tokens are short-lived (eight minutes). Connections would need to re-authenticate after the upgrade, but they need to do this regularly as part of basic operation.

@alexkalish @jvanderhoof
Hi - we also have the fingerprint of a tenant key in slosilo_keystore table that is not expired and not getting updated unless done by rake/manual task...

@alexkalish @jvanderhoof
Is Dvir's comment affect your request?
Thanks

[alexkalish]

@uCatu @nahumcohen: Based on your comments, it sounds like there is a gap in the upgrade UX for OSS, right? I get that we don't have a documented upgrade process now, but we at least need to have a feature in the product that allows users to upgrade. What would you recommend?

[jvanderhoof]

I have only a moderately high level of understanding of the details of Slosilo and it's role in sessions. If you have concerns (which it sounds like you do), I'd strongly recommend validating the proposed change between versions. If there are any issues, you'll need to document the upgrade path as @alexkalish suggested.

[nahumcohen]

@uCatu @nahumcohen: Based on your comments, it sounds like there is a gap in the upgrade UX for OSS, right? I get that we don't have a documented upgrade process now, but we at least need to have a feature in the product that allows users to upgrade. What would you recommend?

@alexkalish We agree that the OSS upgrade should be documented/have a feature.
Guess that this a slosilo PR and the repo is not the place to discuss it.

[nahumcohen]

I have only a moderately high level of understanding of the details of Slosilo and it's role in sessions. If you have concerns (which it sounds like you do), I'd strongly recommend validating the proposed change between versions. If there are any issues, you'll need to document the upgrade path as @alexkalish suggested.

@jvanderhoof We are taking the OSS upgrade process offline with @alexkalish.
Regarding only this PR scope, what are the steps required to approve (technical wise) this PR?

[dividedmind]

FWIW, hash strength here doesn't matter at all. The hash is only there as a hint to identify a key instead of trying them all one by one; if the key doesn't match (regardless of whether the hash matches), the signature verification will simply fail. If there is more than one key that matches the hash, all of them will be tried (as far as I remember). AFAIR I picked MD5 just because it makes for a shorter token. Could just as well have been a random string, or a random number, or anything, or nothing. Using a hash makes it easier because you don't have to store an additional identifier along with the key.

[shaharglazner]

@dividedmind
We are moving to SHA256 not because of a specific security concern, but due to usage of FIPS-compliant OpenSSL which blocks every usage of MD5. Without changing that algorithm, the application crash.

[dividedmind]

Ah, fair, that makes sense, thanks @shaharglazner! In this case perhaps you should consider simply dropping in Ruby's built-in Digest::MD5 which doesn't depend on OpenSSL.

[shaharglazner]

@dividedmind
So actually Digest::MD5 somehow depend on OpenSSL.
For example, look at what happening when we enable fips and then try to use Digest:

irb(main):002:0> require 'openssl'
=> true
irb(main):003:0> OpenSSL.fips_mode = true
=> true
irb(main):004:0> require 'digest'
=> false
irb(main):005:0> Digest::MD5.hexdigest('a')
md5_dgst.c(75): OpenSSL internal error, assertion failed: Low level API call to digest MD5 forbidden in FIPS mode!
Aborted

[alexkalish]

@nahumcohen: Leaving the OSS upgrade changes/documentation for later is fine with me, assuming that it doesn't require changes should be made in this branch. Could you please create an issue in cyberark/conjur for that work, if one doesn't already exist, and link it here? Thanks!

[nahumcohen]

@nahumcohen: Leaving the OSS upgrade changes/documentation for later is fine with me, assuming that it doesn't require changes should be made in this branch. Could you please create an issue in cyberark/conjur for that work, if one doesn't already exist, and link it here? Thanks!

@alexkalish: Added existing issue in the PR description

[jvanderhoof]

@nahumcohen, can you please link to the passing tests on cyberark/conjur (instructions are above)? I need to see those tests before this change can be merged.

[nahumcohen]

@nahumcohen, can you please link to the passing tests on cyberark/conjur (instructions are above)? I need to see those tests before this change can be merged.

@jvanderhoof
OSS branch consist of slosilo change:
https://jenkins.conjur.net/job/cyberark--conjur/job/openssl-1.0.2final/29/
Failures in between are related to base image or to websocket-extensions upgrade.
https://jenkins.conjur.net/job/cyberark--conjur/job/openssl-1.0.2final/34/

[jvanderhoof]

Looks good!

[aloncarmel111]

LGTM