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

Update generate-certificate.sh to write certs etc. to current dir #79

Closed
wants to merge 1 commit into from

Conversation

ghenry
Copy link

@ghenry ghenry commented Jul 12, 2024

Caught me out a few times now, so submitting this PR.

Thanks!

@ghenry
Copy link
Author

ghenry commented Jul 12, 2024

This works as expected now:

RUST_BACKTRACE=1 cargo run --example server -- 127.0.0.1:8000 --cert scripts/cert.pem --key scripts/server.key.pem 
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/examples/server '127.0.0.1:8000' --cert scripts/cert.pem --key scripts/server.key.pem`

@djc
Copy link
Member

djc commented Jul 12, 2024

I don't consider generate-certificate.sh to be a public API, so I don't think we want to support it by catering to your needs (which, as discussed in #76, are definitely not in scope for this script).

@djc djc closed this Jul 12, 2024
@ghenry
Copy link
Author

ghenry commented Jul 12, 2024

For writing the certs out to a directory you're working in? That's not a very complicated need and should have been the default IMHO. I think of this as a bug fix, but your call obviously.

@djc
Copy link
Member

djc commented Jul 12, 2024

I mean that you should not be using this script, as it is strictly intended for this repository's maintainers to help maintain the test suite, and not for anything else. "bug fix" implies that there is expected behavior to verify the actual behavior against, and I am arguing the behavior is unspecified. If you want to make changes, please maintain your own copy.

@ghenry
Copy link
Author

ghenry commented Jul 12, 2024

Understood, thanks.

@cpu
Copy link
Member

cpu commented Jul 12, 2024

@ghenry Thanks for the PR. As I mentioned on the other issue thread I think we should delete that script entirely. It's never even been used for this repo's code. I've put up a PR doing so and replaced it with a pure Rust alternative for generating our test certificates: #80 You might find the new rcgen based code useful as inspiration but echoing djc's point we wouldn't consider updates to make that integration test more generally useful outside of this repo. It's purely scaffolding for our own testing purposes.

If you're especially keen to have your bash script fix live on it looks like tokio/tls (for the non-Rustls maintained native-tls) still uses the script. Perhaps that repo would take a PR (though other updates to the same area look to be stale: tokio-rs/tls#153)

@ghenry
Copy link
Author

ghenry commented Jul 12, 2024

Thanks for the helpful comments @cpu Much appreciated.

I'll take a look. Certainly not a great first impression for a contribution, but it's my own fault for not asking @djc or similar first, so that's on me.

Have a great weekend!

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