Conversation
vic1707
force-pushed
the
yescrypt
branch
2 times, most recently
from
aded7bf to
c957c0e
Compare
|
Edit: I edited the PR to have both Yescrypt and YescryptMCF functions, the first one to stay consistent with other crypto functions, the second because it's a format common enough in config files 🙃 |
vic1707
force-pushed
the
yescrypt
branch
2 times, most recently
from
67e85b3 to
8d83b8d
Compare
|
This pull request is stale because it has been open for 60 days with If it's still relevant, one of the following will remove the stale
|
|
🥲 |
|
While waiting for this, I made #!/usr/bin/env rust-script
//! ```cargo
//! [dependencies]
//! anyhow = { version = "1.0.100", default-features = false }
//! clap = { version = "4.5.53", default-features = false, features = ["derive", "std"] }
//! clap-stdin = { version = "0.8.0", default-features = false }
//! pbkdf2 = { version = "0.13.0-rc.2", default-features = false, features = ["simple"] }
//! rand = { version = "0.9.2", default-features = false, features = ["alloc", "thread_rng"]}
//! yescrypt = { version = "0.1.0-rc.0", default-features = false, features = ["simple"] }
//! ```
use clap::{Parser, ValueEnum};
use clap_stdin::MaybeStdin;
use pbkdf2::{
password_hash::{self, PasswordHasher, PasswordVerifier, SaltString},
Pbkdf2,
};
const DEFAULT_PWD_LENGTH: usize = 64;
const PBKDF2_ALGO: password_hash::Ident = password_hash::Ident::new_unwrap("pbkdf2-sha512");
const PBKDF2_PARAMS: ::pbkdf2::Params = ::pbkdf2::Params {
rounds: 600_000, // Authelia's default == 310_000
output_length: 64, // Authelia's default == 72, crate only supports <=64
};
#[derive(Parser)]
struct Cli {
#[arg(long, value_enum)]
algo: Algo,
#[arg(short, long, default_value_t = DEFAULT_PWD_LENGTH)]
length: usize,
#[arg()]
input: Option<MaybeStdin<String>>,
}
fn main() -> anyhow::Result<()> {
let Cli {
input,
algo,
length,
} = Cli::parse();
let salt = SaltString::generate();
let password = input
.map(|v| v.into_inner())
.unwrap_or_else(|| random_string(length));
let hash = &algo.hash_and_verify(password.as_bytes(), &salt)?;
// dbg!(password, salt, hash);
print!("{}", hash);
Ok(())
}
fn random_string(lenght: usize) -> String {
use rand::distr::{Alphanumeric, SampleString};
Alphanumeric.sample_string(&mut ::rand::rng(), lenght)
}
#[derive(ValueEnum, Clone)]
enum Algo {
Pbkdf2,
Yescrypt,
}
impl Algo {
fn hash_and_verify(&self, password: &[u8], salt: &SaltString) -> anyhow::Result<String> {
match self {
Self::Yescrypt => {
let hash = ::yescrypt::yescrypt(
password,
salt.as_str().as_bytes(),
&::yescrypt::Params::default(),
)?;
::yescrypt::yescrypt_verify(password, &hash)?;
Ok(hash)
}
Self::Pbkdf2 => {
let hcp_hash = Pbkdf2.hash_password_customized(
password,
Some(PBKDF2_ALGO),
None,
PBKDF2_PARAMS,
salt,
)?;
Pbkdf2.verify_password(password, &hcp_hash)?;
// dirty convert to mcf format
Ok(format!(
"${}${}${}${}",
hcp_hash.algorithm,
PBKDF2_PARAMS.rounds,
hcp_hash.salt.expect("Pbkdf2 no salt"),
hcp_hash.hash.expect("Pbkdf2 no hash"),
).replace('+', "."))
}
}
}
}Usable within gomplate via plugins (requires rust-script & rust/cargo itself) plugins:
yescrypt:
cmd: .conf/hash
args:
- --algo=yescrypt
pbkdf2:
cmd: .conf/hash
args:
- --algo=pbkdf2Then you use them as |
|
Apologies for the silence on this one - I don't have much free time to review PRs. I'll try to get to it over the coming week. First, however, can you please rebase to resolve the conflicts? |
vic1707
force-pushed
the
yescrypt
branch
2 times, most recently
from
87d8156 to
8a78d0e
Compare
|
@hairyhenderson Rebase done, one tais is failing but I believe it's from main 🤔 |
|
@hairyhenderson rebase done, one test still failling but I believe it's from main |
go.mod
Outdated
| // is merged | ||
| require github.com/hairyhenderson/yaml v0.0.0-20220618171115-2d35fca545ce | ||
|
|
||
| require github.com/openwall/yescrypt-go v1.0.0 |
There was a problem hiding this comment.
this should ideally go into the require block above with all the other direct dependencies
internal/funcs/crypto.go
Outdated
| msg = toBytes(args[1]) | ||
| default: | ||
| return nil, nil, fmt.Errorf("wrong number of args: want 2 or 3, got %d", len(args)) | ||
| return nil, nil, fmt.Errorf("wrong number of args: want 1 or 2, got %d", len(args)) |
There was a problem hiding this comment.
thanks for fixing this, though maybe best to go in a separate PR since it's completely unrelated to this feature
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the Yescrypt password hashing algorithm to gomplate's crypto functions. Yescrypt is a modern, memory-hard key derivation function designed as an extension of scrypt, providing resistance to GPU/ASIC attacks. This addresses issue #2384, which requested this functionality for use with Fedora CoreOS installations.
Changes:
- Adds two new experimental crypto functions:
crypto.Yescrypt(low-level key derivation) andcrypto.YescryptMCF(password hashing with MCF format) - Adds comprehensive test coverage for both new functions
- Adds documentation for both functions in YAML and Markdown formats
- Includes a minor bug fix for an incorrect error message in
parseAESArgs
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/funcs/crypto.go | Implements Yescrypt and YescryptMCF functions with vendored base64 encoding helpers; includes unrelated bug fix for AES error message |
| internal/funcs/crypto_test.go | Adds comprehensive test cases for both new functions including edge cases and verification |
| go.mod | Adds dependency on github.com/openwall/yescrypt-go v1.0.0 |
| go.sum | Adds checksums for the new dependency |
| docs/content/functions/crypto.md | Documents both new functions with usage examples and parameter descriptions |
| docs-src/content/functions/crypto.yml | Source documentation for generating crypto.md |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Yescrypt - | ||
| func (CryptoFuncs) Yescrypt(password, salt, cost, blockSize, keylen any) (string, error) { | ||
| N, err := conv.ToInt(cost) | ||
| if err != nil { | ||
| return "", fmt.Errorf("cost must be an integer: %w", err) | ||
| } | ||
| r, err := conv.ToInt(blockSize) | ||
| if err != nil { | ||
| return "", fmt.Errorf("blockSize must be an integer: %w", err) | ||
| } | ||
| k, err := conv.ToInt(keylen) | ||
| if err != nil { | ||
| return "", fmt.Errorf("keylen must be an integer: %w", err) | ||
| } | ||
|
|
||
| key, err := yescrypt.Key(toBytes(password), toBytes(salt), N, r, 1, k) | ||
| return fmt.Sprintf("%02x", key), err | ||
| } |
There was a problem hiding this comment.
The Yescrypt function is missing the experimental check. Since this function is marked as experimental in the documentation, it should check if experimental mode is enabled. The function should use a pointer receiver (f *CryptoFuncs) instead of (CryptoFuncs) to access f.ctx, and add if err := checkExperimental(f.ctx); err != nil { return "", err } at the beginning of the function, similar to other experimental crypto functions like RSAEncrypt and DecryptAES.
| // YescryptMCF - | ||
| func (CryptoFuncs) YescryptMCF(args ...any) (string, error) { | ||
| cost := 14 | ||
| blockSize := 8 | ||
| salt, _ := RandomFuncs{}.AlphaNum(16) | ||
| input := "" | ||
| var err error | ||
|
|
||
| switch len(args) { | ||
| case 1: | ||
| input = conv.ToString(args[0]) | ||
| case 3: | ||
| cost, err = conv.ToInt(args[0]) | ||
| if err != nil { | ||
| return "", fmt.Errorf("yescrypt cost must be an integer: %w", err) | ||
| } | ||
|
|
||
| blockSize, err = conv.ToInt(args[1]) | ||
| if err != nil { | ||
| return "", fmt.Errorf("yescrypt blockSize must be an integer: %w", err) | ||
| } | ||
|
|
||
| input = conv.ToString(args[2]) | ||
| case 4: | ||
| cost, err = conv.ToInt(args[0]) | ||
| if err != nil { | ||
| return "", fmt.Errorf("yescrypt cost must be an integer: %w", err) | ||
| } | ||
|
|
||
| blockSize, err = conv.ToInt(args[1]) | ||
| if err != nil { | ||
| return "", fmt.Errorf("yescrypt blockSize must be an integer: %w", err) | ||
| } | ||
|
|
||
| salt = conv.ToString(args[2]) | ||
| input = conv.ToString(args[3]) | ||
| default: | ||
| return "", fmt.Errorf("wrong number of args: want 1 or 4, got %d", len(args)) | ||
| } | ||
|
|
||
| // yescrypt requires | ||
| // - cost ∈ [10, 18] | ||
| // - blockSize ∈ [1, 32] | ||
| N := yescryptItoa64[(cost-1)&0x3f] | ||
| r := yescryptItoa64[(blockSize-1)&0x3f] | ||
| saltB64 := yescryptEncode64([]byte(salt)) | ||
| settings := fmt.Sprintf("$y$j%c%c$%s", N, r, saltB64) | ||
|
|
||
| hash, err := yescrypt.Hash([]byte(input), []byte(settings)) | ||
| return string(hash), err | ||
| } |
There was a problem hiding this comment.
The YescryptMCF function is missing the experimental check. Since this function is marked as experimental in the documentation, it should check if experimental mode is enabled. The function should use a pointer receiver (f *CryptoFuncs) instead of (CryptoFuncs) to access f.ctx, and add if err := checkExperimental(f.ctx); err != nil { return "", err } at the beginning of the function, similar to other experimental crypto functions like RSAEncrypt and DecryptAES.
docs/content/functions/crypto.md
Outdated
| |------|-------------| | ||
| | `cost` | _(optional)_ the cost parameter (log₂ of the iteration count) - integer from `10` to `18` - defaults to `14` | | ||
| | `blockSize` | _(optional)_ the block size parameter (`r`) - integer from `1` to `32` - defaults to `8` | | ||
| | `salt` | _(optional)_ the salt string used in hashing - defaults to a random alphanumeric string of length 10 | |
There was a problem hiding this comment.
The documentation states the default salt is "a random alphanumeric string of length 10", but the code actually generates a 16-character string with RandomFuncs{}.AlphaNum(16) (see line 224 in crypto.go). The documentation should be updated to say "length 16" instead of "length 10".
| | `salt` | _(optional)_ the salt string used in hashing - defaults to a random alphanumeric string of length 10 | | |
| | `salt` | _(optional)_ the salt string used in hashing - defaults to a random alphanumeric string of length 16 | |
| description: the block size parameter (`r`) - integer from `1` to `32` - defaults to `8` | ||
| - name: salt | ||
| required: false | ||
| description: the salt string used in hashing - defaults to a random alphanumeric string of length 10 |
There was a problem hiding this comment.
The documentation states the default salt is "a random alphanumeric string of length 10", but the code actually generates a 16-character string with RandomFuncs{}.AlphaNum(16) (see line 224 in crypto.go). The documentation should be updated to say "length 16" instead of "length 10".
| description: the salt string used in hashing - defaults to a random alphanumeric string of length 10 | |
| description: the salt string used in hashing - defaults to a random alphanumeric string of length 16 |
internal/funcs/crypto.go
Outdated
| salt, _ := RandomFuncs{}.AlphaNum(16) | ||
| input := "" | ||
| var err error |
There was a problem hiding this comment.
The error from RandomFuncs{}.AlphaNum(16) is silently ignored using the blank identifier. If salt generation fails, this could lead to an empty salt being used, which would be a security issue. The error should be checked and propagated, e.g., salt, err := RandomFuncs{}.AlphaNum(16); if err != nil { return "", fmt.Errorf("failed to generate salt: %w", err) }.
| salt, _ := RandomFuncs{}.AlphaNum(16) | |
| input := "" | |
| var err error | |
| salt, err := RandomFuncs{}.AlphaNum(16) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to generate salt: %w", err) | |
| } | |
| input := "" |
| // yescrypt requires | ||
| // - cost ∈ [10, 18] | ||
| // - blockSize ∈ [1, 32] | ||
| N := yescryptItoa64[(cost-1)&0x3f] | ||
| r := yescryptItoa64[(blockSize-1)&0x3f] | ||
| saltB64 := yescryptEncode64([]byte(salt)) | ||
| settings := fmt.Sprintf("$y$j%c%c$%s", N, r, saltB64) |
There was a problem hiding this comment.
The function doesn't validate that cost is in the range [10, 18] and blockSize is in the range [1, 32] before constructing the settings string and calling yescrypt.Hash. While the yescrypt library will validate these and return an error, it would be more maintainable and user-friendly to validate early with clear, gomplate-specific error messages. For example, add validation like: if cost < 10 || cost > 18 { return "", fmt.Errorf("cost must be between 10 and 18, got %d", cost) } and similar for blockSize.
There was a problem hiding this comment.
this is validated by the yescrypt lib itself, should we still validate ourselves?
| func TestYescryptMCF(t *testing.T) { | ||
| t.Parallel() | ||
| c := testCryptoNS() | ||
|
|
||
| in := "foo" | ||
|
|
||
| t.Run("no arg default", func(t *testing.T) { | ||
| t.Parallel() | ||
| actual, err := c.YescryptMCF(in) | ||
| require.NoError(t, err) | ||
| assert.True(t, strings.HasPrefix(actual, "$y$jB5$")) | ||
| }) | ||
|
|
||
| t.Run("cost less than min", func(t *testing.T) { | ||
| t.Parallel() | ||
| _, err := c.YescryptMCF(9, 8, "salt", in) | ||
| require.ErrorContains(t, err, "N out of supported range") | ||
| }) | ||
|
|
||
| t.Run("blockSize less than min", func(t *testing.T) { | ||
| t.Parallel() | ||
| _, err := c.YescryptMCF(14, 0, "salt", in) | ||
| require.ErrorContains(t, err, "r out of supported range") | ||
| }) | ||
|
|
||
| t.Run("custom salt appears", func(t *testing.T) { | ||
| t.Parallel() | ||
| salt := "abc123" | ||
| actual, err := c.YescryptMCF(14, 8, salt, in) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, actual, string(yescryptEncode64([]byte(salt)))) | ||
| }) | ||
|
|
||
| t.Run("wrong arg count", func(t *testing.T) { | ||
| t.Parallel() | ||
| _, err := c.YescryptMCF() | ||
| require.Error(t, err) | ||
| _, err = c.YescryptMCF(14, in) // 2 args | ||
| require.Error(t, err) | ||
| }) | ||
|
|
||
| t.Run("hash changes with different salts", func(t *testing.T) { | ||
| t.Parallel() | ||
| a1, _ := c.YescryptMCF(in) | ||
| a2, _ := c.YescryptMCF(in) | ||
| assert.NotEqual(t, a1, a2) | ||
| }) | ||
|
|
||
| t.Run("hash verifies", func(t *testing.T) { | ||
| t.Parallel() | ||
| hash, err := c.YescryptMCF(14, 8, "salt", in) | ||
| require.NoError(t, err) | ||
| parts := strings.Split(hash, "$") | ||
| require.GreaterOrEqual(t, len(parts), 4) | ||
| settings := strings.Join(parts[:4], "$") // "$y$jF7$<salt-b64>" | ||
| verify, err := yescrypt.Hash([]byte(in), []byte(settings)) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, hash, string(verify)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
There is no test case for the 3-argument case of YescryptMCF, which handles cost, blockSize, input (see line 231-242 in crypto.go). A test case should be added to verify this code path works correctly, e.g., c.YescryptMCF(14, 8, "password") and verify the result has the expected cost and blockSize encoded in the hash prefix.
internal/funcs/crypto.go
Outdated
| salt = conv.ToString(args[2]) | ||
| input = conv.ToString(args[3]) | ||
| default: | ||
| return "", fmt.Errorf("wrong number of args: want 1 or 4, got %d", len(args)) |
There was a problem hiding this comment.
The error message says "want 1 or 4" but the function actually accepts 1, 3, or 4 arguments (case 3 handles cost, blockSize, and input). The error message should be updated to say "want 1, 3, or 4" to accurately reflect the accepted argument counts.
| return "", fmt.Errorf("wrong number of args: want 1 or 4, got %d", len(args)) | |
| return "", fmt.Errorf("wrong number of args: want 1, 3, or 4, got %d", len(args)) |
|
Hi @hairyhenderson, |
Fixes: #2384
Hi, tried to implement it, let me know what you think 😁
I might also try to domaybe laterArgon2&Argon2Idwhile I'm at it.Note: I'm learning go, my code is probably not the prettiest/idiomatic, please be kind 🙏