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

feat: add support for proxy auth #88

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

austinsasko
Copy link

Support added for proxy authentication. This does not add tests to validate proxy / noproxy use cases, nor is this validated against non proxy environments, but is fully functional in a corporate proxy-backed environment.

Co-Authors: @cbacken / https://github.com/cbacken

@austinsasko
Copy link
Author

Also note the Cargo.lock was not committed as part of this PR.

@austinsasko
Copy link
Author

@roblabla I left out a comma when trying to copy and paste from our internal environment (oops) and ended up failing the build workflows. Fixed now

Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

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

This PR has been bit rotted against our main branch. I attempted to rebase it. But it isn't clear to me how the new proxy code works in aws-smithy 0.57, which refactored things: smithy-lang/smithy-rs#3022.

I'm also a bit concerned about the presence of the ~2 year old hyper-proxy 0.9.1. It appears this version is pulling in very old versions of various crates, including an older version of ring and rustls.

I may look at this again in a day or two with a fresh brain. But if the original authors want to clean it up and get it working with aws-smithy 0.57, it would be greatly appreciated.

The PR can be rebased against the main branch, as the certificates changes related to this PR were merged into main a few hours ago.

apple-codesign/src/notarization.rs Outdated Show resolved Hide resolved
apple-codesign/src/notarization.rs Outdated Show resolved Hide resolved
roblabla and others added 4 commits June 5, 2024 12:56
This allows using the same version of rustls betwen apple-codesign and
the AWS SDK, which allows enabling the native-tls features of rustls
inside the S3 client.
@austinsasko austinsasko changed the base branch from native-tls-apple-codesign to main June 5, 2024 17:02
@austinsasko austinsasko force-pushed the feat/support-proxy-auth branch from 3da04ae to 233d114 Compare June 5, 2024 17:23
@austinsasko
Copy link
Author

@indygreg I think I was able to clean it up and apply your feedback. Please let me know

@austinsasko
Copy link
Author

It seems I messed up a few things in the merge conflict. Will push an update that I tested the fix on tomorrow.

@austinsasko
Copy link
Author

Tested build, run, and OS X + Ubuntu binary locally from a corp proxy environment and non-corp proxy environment. Success on all fronts.

@austinsasko austinsasko requested a review from indygreg June 12, 2024 20:28
@austinsasko
Copy link
Author

@indygreg Are these workflows failing due to issues with my PR, do you have any insight into that?

apple-codesign/Cargo.toml Outdated Show resolved Hide resolved
@austinsasko
Copy link
Author

Updated @roblabla / @indygreg

@austinsasko
Copy link
Author

Looks like it cleared some checks, but some are still failing, any idea?

Thanks @roblabla / @indygreg

@austinsasko
Copy link
Author

I am able to build this locally on 3 different OS, not sure what I can do to make the CI succeed here. @roblabla @indygreg

@austinsasko
Copy link
Author

Is there something obvious I am missing on my end? @roblabla / @indygreg hoping to follow up, would be a great add-on. Thanks

@roblabla
Copy link
Collaborator

The nightly failure can probably be fixed by updating the time dependency, with cargo update -p time.

The 1.70 failure, I'm not sure. Might be worth updating the smithy crates, see if that fixes the issues?

@austinsasko
Copy link
Author

That helped with 3 of the builds, thanks! will try what you suggested on the other 6, feels we are so close!

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.

4 participants