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

WIP: Upgrade versions of RustCrypto to pre #86

Draft
wants to merge 3 commits into
base: wiktor/support-certs-everywhere
Choose a base branch
from

Conversation

wiktor-k
Copy link
Owner

@baloo sorry to interrupt but maybe you'll have a clue on why these tests are failing.

This is a one-commit extension to #85 which only upgrades RustCrypto crates to latest versions (to pull changes such as RustCrypto/SSH#233) (and also it removes some example code that I didn't want to update).

The tests fail with error: ssh_agent_lib::proto::error::ProtoError - SSH signature error: signature error: unsupported algorithm: ssh-ed25519 but there are no functional changes, only the bump in versions. Is this a regression in the RustCrypto crates or am I holding it wrong?

Thanks in advance for your help! 🙇

@jcspencer
Copy link
Collaborator

@wiktor-k potentially related to this commit? RustCrypto/SSH@0782a5c

@baloo
Copy link
Collaborator

baloo commented Jul 31, 2024

interestingly ... the tests would fail when running on my machine, but they work in CI?

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests src/lib.rs (target/debug/deps/ssh_agent_lib-7312c3c9364a567c)

running 2 tests
test proto::extension::constraint::tests::parse_destination_constraint ... FAILED
test proto::extension::message::tests::parse_bind ... ok

failures:

---- proto::extension::constraint::tests::parse_destination_constraint stdout ----
Destination constraint: RestrictDestination { constraints: [DestinationConstraint { from: HostTuple { username: "", hostname: "", keys: [] }, to: HostTuple { username: "", hostname: "github.com", keys: [KeySpec { keyblob: Ed25519(Ed25519PublicKey([227, 42, 170, 121, 21, 206, 185, 180, 73, 209, 186, 80, 234, 42, 40, 187, 26, 110, 1, 249, 11, 218, 36, 90, 45, 29, 135, 105, 125, 24, 162, 101])), is_ca: false }, KeySpec { keyblob: Rsa(RsaPublicKey { e: Mpint(010001), n: Mpint(00A3EE774DC50A3081C4278EC85C2EBA8F1228A9867B7E5534EF0CFEA61C12FD8F568D52463851ED60BF09C62D594E846798AE765A32044AEBE3CA0945DA0DB0BBAAD6D6F2022484BEDA182B0EAFF0B9E9224CCBF04265FC5DD675B300EC520CF815B267AB38161F36A96D57DFE1582A81CB020D211FB974883A25327BDA9704A448DC6205E41366041575752479EC2A06CB58D96149CA9BD949B2464432CAD44BB4BFB7F131B193109F9663BEE59F02492358EC689D8CC219ED0E333230369F59C6AE54C3933C030ACC3EC2A14F190035EFD7277C658E59156BBA3D7ACFA5F2BF1BE32706F3D30419EF95CAE6D2926FB14DC9E204B384D3E2393E4B87613DE0140B9CBE6C3622AD880CE060BBB849F3B67672695590EC1DFCD402B841DAF0B79D59A84C4A6D0A5350D9FE123AA84F0BEA363E24AB1E505022344E14BF6243B12425E63D45996E18E90A0E7A8BED9A07A0A62B6246867E7B2B99A3D0C35D057038FD69F01FA5E83D62732B9372BB6CC1DE7019A7E4B986942CFA9D6F375FF0B239) }), is_ca: false }, KeySpec { keyblob: Ecdsa(NistP256(EncodedPoint(Uncompressed { x: Array([73, 138, 72, 67, 99, 64, 71, 179, 58, 108, 100, 100, 204, 187, 162, 146, 160, 192, 80, 125, 158, 75, 121, 97, 26, 216, 50, 51, 110, 27, 147, 124]), y: Array([238, 228, 96, 131, 160, 139, 173, 186, 57, 192, 7, 83, 255, 46, 175, 210, 98, 149, 209, 77, 176, 209, 102, 118, 96, 31, 254, 249, 58, 104, 114, 72]) }))), is_ca: false }] } }] }
thread 'proto::extension::constraint::tests::parse_destination_constraint' panicked at src/proto/extension/constraint.rs:313:38:
error: ssh_agent_lib::proto::error::ProtoError - SSH encoding error: length invalid
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    proto::extension::constraint::tests::parse_destination_constraint

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

EDIT: because the CI did not run.

wiktor-k and others added 2 commits July 31, 2024 09:31
@baloo
Copy link
Collaborator

baloo commented Jul 31, 2024

(rebased)

@wiktor-k
Copy link
Owner Author

wiktor-k commented Aug 1, 2024

potentially related to this commit? RustCrypto/SSH@0782a5c

Excellent detective work, folks! 🙇

Sadly this means we still can't build the thing without patched sources 😢

What would be the way to proceed here @baloo? Wait until the RustCrypto crates stabilize a bit?

I'm wondering if it makes sense to merge #85 as is (I think @jcspencer is leaning on this approach), release 0.5 (with -pre in between). If RustCrypto stabilizes we can fix that in 0.5.1... or maybe I'm missing something? 🤔

@baloo
Copy link
Collaborator

baloo commented Aug 1, 2024

This will require at least a pre-release of ed25519-dalek (and others in the repo, but I don't have access there) and a subsequent release of ssh-* after that (that I can do).

I have no idea what the timeline is in regard to ed25519.
We can go and use the non-pre-release, non-patched sources, and release a 0.5 here then bump and release a 0.6 (nice thing about numbers is we have a fairly unlimited supply of those).

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