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

allow to skip cert validation for internal test situations #565

Merged
merged 3 commits into from
Sep 28, 2023
Merged

allow to skip cert validation for internal test situations #565

merged 3 commits into from
Sep 28, 2023

Conversation

raffaeleragni
Copy link

Adding the possibility to setup Reqwest client with ignore certificate mode.

This feature is used in cases https is used for non terminated proxies, but when testing internal infrastructure setups eg. when self signed or dev certificates are being used.

This is what I currently need to be able to use goose in the setup I have to load test.

Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

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

Functionally this is great, thanks!

Please add the missing comments and update as flagged.

It would be great to add test coverage as well, but is not required.

src/config.rs Outdated
@@ -195,6 +195,9 @@ pub struct GooseConfiguration {
/// Follows base_url redirect with subsequent requests
#[options(no_short)]
pub sticky_follow: bool,
/// Allows to test internally when https validation is not required
Copy link
Member

Choose a reason for hiding this comment

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

How about documenting this as:
Disables validation of https certificates

We can better document in the goose book (and it can happen in a followup PR).

src/config.rs Outdated
@@ -332,6 +335,8 @@ pub(crate) struct GooseDefaults {
pub websocket_host: Option<String>,
/// An optional default for port WebSocket Controller listens on.
pub websocket_port: Option<u16>,
/// Allow for internal testing when no certificates are available but still using https
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same format as the above:
An optional default for not validating https certificates.

src/config.rs Show resolved Hide resolved
@@ -2318,6 +2318,7 @@ pub(crate) fn create_reqwest_client(
.timeout(Duration::from_millis(timeout))
// Enable gzip unless `--no-gzip` flag is enabled.
.gzip(!configuration.no_gzip)
.danger_accept_invalid_certs(configuration.accept_invalid_certs)
Copy link
Member

Choose a reason for hiding this comment

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

Please include a comment, for example:

// Validate https certificates unless --accept_invalid_certs is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, sorry, I think that's actually --accept-invalid-certs -- can you test and confirm, and fix if necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@raffaeleragni
Copy link
Author

Changed the comments

Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

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

I think I gave you a bad example, and the _ needs to be converted to -?

src/config.rs Show resolved Hide resolved
@@ -2318,6 +2318,7 @@ pub(crate) fn create_reqwest_client(
.timeout(Duration::from_millis(timeout))
// Enable gzip unless `--no-gzip` flag is enabled.
.gzip(!configuration.no_gzip)
.danger_accept_invalid_certs(configuration.accept_invalid_certs)
Copy link
Member

Choose a reason for hiding this comment

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

Whoops, sorry, I think that's actually --accept-invalid-certs -- can you test and confirm, and fix if necessary?

@raffaeleragni
Copy link
Author

Ok, changed

Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

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

Thanks for the new feature!

@@ -2318,6 +2318,7 @@ pub(crate) fn create_reqwest_client(
.timeout(Duration::from_millis(timeout))
// Enable gzip unless `--no-gzip` flag is enabled.
.gzip(!configuration.no_gzip)
.danger_accept_invalid_certs(configuration.accept_invalid_certs)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@jeremyandrews jeremyandrews merged commit 6d0f111 into tag1consulting:main Sep 28, 2023
2 checks passed
@raffaeleragni raffaeleragni deleted the accept_invalid_certs branch September 28, 2023 14:59
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