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

remove disableStrictSsl param from [jenkins] #6887

Merged
merged 2 commits into from Aug 18, 2021

Conversation

chris48s
Copy link
Member

After posting #4655 (comment) yesterday, I started looking at the strictSSL/proxy thing and on digging into it.. it might be a non issue. There are two services where we currently do something with the strictSSL param, and I think they're both now pointless. In this PR: jenkins

I was looking into how to test this param and I realised this param has actaully been broken for a very long time. Any attempt to use this will throw "strict ssl is required" e.g: https://img.shields.io/jenkins/build.svg?disableStrictSSL&jobUrl=https%3A%2F%2Fjenkins-oss.wixpress.com%2Fjob%2Fmulti-detox-master In order to make it work, you have to remove

this.enforceStrictSsl(requestParams)

We broke this over a year ago in #4729 and nobody has raised an issue about it. Given this was never documented anywhere and has been broken for over a year without anyone noticing, I reckon we can just quietly drop this param rather than fixing it. Clearly nobody is using it.

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Aug 12, 2021
@shields-ci
Copy link

shields-ci commented Aug 12, 2021

Warnings
⚠️

📚 Remember to ensure any changes to config.private in services/jenkins/jenkins-base.js are reflected in the server secrets documentation

⚠️ This PR modified service code for jenkins but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 77aaede

@calebcartwright
Copy link
Member

Relevant originating issue for reference was #1956. I still think the self-hosting scenarios and the common self-signed cert usage, particularly within intranets, is something we need to take into consideration.

@chris48s
Copy link
Member Author

chris48s commented Aug 12, 2021

This (disableStrictSsl for jenkins) has also been broken for self-hosted installs since ~March 2020. As far as I can tell, the only way you'd have a self-hosted install where this works is if you are currently running a pre-March 2020 install or customising by running your own fork. Is there another scenario I'm missing here? (entirely possible)

@calebcartwright
Copy link
Member

where this works is if you are currently running a pre-March 2020 install or customising by running your own fork. Is there another scenario I'm missing here? (entirely possible)

That's correct, but not as far fetched as it may sound; we can't assume self-hosters redploy with an updated instance on a regular basis because there's rarely motivation for them to do so. The concern would be someone subsequently needing an update (security patch, general rehydration, etc.) to discover the issue.

@calebcartwright
Copy link
Member

And to clarify, I'm not necessarily opposed to this/am undecided, just want to highlight that the absence of reported issues isn't as strong a point of evidence as it typically is IMO due to the self-hosting vector

@chris48s
Copy link
Member Author

Cool - just wanted to check there isn't another case I am not seeing here.

@calebcartwright
Copy link
Member

Having thought on this a bit more, I feel like there are viable options for folks using a self-hosted Shields server. Seems reasonable to assume that in many corporate/intranet scenarios that there would be a corporate-level private CA in use, and even in other cases it wouldn't be that difficult for someone to create their own and add it to both their Jenkins and Shields server.

The Endpoint badge could always be used as an intermediary where they could disable the strict SSL check on the call to Jenkins from the endpoint.

Obviously that story is more tricky for folks trying to use Shields.io (other than the Endpoint badge), but as discussed above the ability to disable the check with Shields.io has been broken for a long time already anyway.

I'm feeling more confident about our ability to move ahead with this 👍

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6887 August 14, 2021 20:38 Inactive
Copy link
Member

@calebcartwright calebcartwright 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've managed to talk myself into this 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants