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

Add a CRD field to replace InsecureSkipVerify=true #1081

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Apr 2, 2024

Test image: docker.io/bdunne/manageiq-operator:ssl_verify
Add optional CR value OIDCOAuthIntrospectionSSLVerify to determine whether or not to verify SSL when fetching the introspection URL. Default to SSLVerify for safety.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

Are you thinking of passing a boolean parameter for passing in InsecureSkipVerify:true but defaulting to false?
Or are you thinking we can just drop it?

@Fryguy
Copy link
Member

Fryguy commented Apr 2, 2024

I had the same q as @kbrock. Do we need a setting for users that might need to set verify false?

@Fryguy Fryguy self-assigned this Apr 10, 2024
@miq-bot
Copy link
Member

miq-bot commented Apr 17, 2024

Checked commit bdunne@485c443 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@bdunne bdunne changed the title [WIP] Remove InsecureSkipVerify Remove InsecureSkipVerify Apr 17, 2024
@bdunne bdunne changed the title Remove InsecureSkipVerify Add a CRD field to replace InsecureSkipVerify=true Apr 23, 2024
customTransport := http.DefaultTransport.(*http.Transport).Clone()
customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: !sslVerify}
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but can we also just not set it? Something like

Suggested change
customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: !sslVerify}
if !sslVerify {
customTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: !sslVerify}
}

Copy link
Member

Choose a reason for hiding this comment

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

Merging anyway.

@Fryguy Fryguy merged commit 5a376e9 into ManageIQ:master Apr 23, 2024
2 checks passed
@bdunne bdunne deleted the ssl_verify branch April 23, 2024 21:40
@Fryguy
Copy link
Member

Fryguy commented May 23, 2024

This PR has been effectively backported to radjabov via the merge of master into radjabov.

bdunne added a commit to bdunne/manageiq-pods that referenced this pull request Jul 2, 2024
Add a CRD field to replace InsecureSkipVerify=true

(cherry picked from commit 5a376e9)

# Conflicts:
#	manageiq-operator/config/crd/bases/manageiq.org_manageiqs.yaml
#	manageiq-operator/pkg/apis/manageiq/v1alpha1/zz_generated.deepcopy.go
@bdunne
Copy link
Member Author

bdunne commented Jul 3, 2024

Unclean backport... Trying in #1128 but tests aren't passing yet.

@Fryguy
Copy link
Member

Fryguy commented Jul 10, 2024

@bdunne I moved this to morphy/yes? so it wouldn't get in the way of other backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants