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

SCRAM-SHA-256 support #257

Merged
merged 5 commits into from Sep 11, 2020
Merged

SCRAM-SHA-256 support #257

merged 5 commits into from Sep 11, 2020

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Sep 11, 2020

resolves #255

def decode(bytes: ByteVector): Option[ServerFirst] =
utf8.decodeValue(bytes.bits).toOption.flatMap {
case Pattern(r, s, i) =>
Some(ServerFirst(r, ByteVector.fromBase64(s).get, i.toInt))

Choose a reason for hiding this comment

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

isn’t there a fromValidBase64?

private val Pattern = """v=([A-Za-z0-9+/]+={0,2})""".r
def decode(bytes: ByteVector): Option[ServerFinal] =
utf8.decodeValue(bytes.bits).toOption.flatMap {
case Pattern(v) =>

Choose a reason for hiding this comment

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

same. i know, the tiniest of nits but seeing get even when its impossible to fail ... -)

ByteVector.view(arr).toBase64
}
clientFirstBareWithNonce(nonce)
}

Choose a reason for hiding this comment

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

i suppose for this kind of thing purity isn’t the point but this i assume isn’t pure with the secure random call - would we want to wrap in an effect?

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 don't know - I originally had this in a F[_]: Sync but that percolates up the call stack. Adding a Random effect seemed overkill. So I simplified to this version.

@mpilquist mpilquist marked this pull request as ready for review September 11, 2020 12:40
@@ -81,6 +81,7 @@ lazy val core = project
"org.scodec" %% "scodec-cats" % "1.0.0",
"com.beachape" %% "enumeratum" % "1.6.1",
"org.tpolecat" %% "natchez-core" % "0.0.12",
"com.ongres.stringprep" % "saslprep" % "1.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is BSD 2-clause licensed and seems safe / unlikely to change or cause dependency issues for folks downstream. The main algorithm is here, which is 100+ lines of unicode manipulation: https://gitlab.com/ongresinc/stringprep/-/blob/development/saslprep/src/main/java/com/ongres/saslprep/SaslPrep.java

Copy link
Member

Choose a reason for hiding this comment

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

👍 seems fine

@codecov-commenter
Copy link

Codecov Report

Merging #257 into master will increase coverage by 0.88%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   83.88%   84.77%   +0.88%     
==========================================
  Files         108      111       +3     
  Lines        1260     1327      +67     
  Branches       27       27              
==========================================
+ Hits         1057     1125      +68     
+ Misses        203      202       -1     
Impacted Files Coverage Δ
...les/core/src/main/scala/net/protocol/Startup.scala 90.62% <89.65%> (-9.38%) ⬇️
...odules/core/src/main/scala/net/message/Scram.scala 95.00% <95.00%> (ø)
...scala/net/message/AuthenticationSASLContinue.scala 50.00% <100.00%> (+50.00%) ⬆️
...in/scala/net/message/AuthenticationSASLFinal.scala 50.00% <100.00%> (+50.00%) ⬆️
...c/main/scala/net/message/SASLInitialResponse.scala 100.00% <100.00%> (ø)
...core/src/main/scala/net/message/SASLResponse.scala 100.00% <100.00%> (ø)
...main/scala/net/message/AuthenticationRequest.scala 46.15% <0.00%> (+23.07%) ⬆️
... and 1 more

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 fa02e0c...fbf991e. Read the comment docs.

Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

Amazing contribution, thank you!

@@ -81,6 +81,7 @@ lazy val core = project
"org.scodec" %% "scodec-cats" % "1.0.0",
"com.beachape" %% "enumeratum" % "1.6.1",
"org.tpolecat" %% "natchez-core" % "0.0.12",
"com.ongres.stringprep" % "saslprep" % "1.1"
Copy link
Member

Choose a reason for hiding this comment

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

👍 seems fine


private def Hi(str: String, salt: ByteVector, iterations: Int): ByteVector = {
val spec = new javax.crypto.spec.PBEKeySpec(str.toCharArray, salt.toArray, iterations, 8 * 32)
val salted = javax.crypto.SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256").generateSecret(spec).getEncoded
Copy link
Member

Choose a reason for hiding this comment

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

Shit almighty man, JCA gives me the heebiejeebies.

@tpolecat tpolecat merged commit 8804dad into typelevel:master Sep 11, 2020
@tpolecat
Copy link
Member

tagged v0.0.20

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.

Implement SCRAM-SHA-256 authentication scheme
4 participants