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

Treat thumbprints as case-insensitive #606

Closed

Conversation

khellang
Copy link

@khellang khellang commented May 3, 2024

Background

See #605

Results

Fixes #605

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

@khellang khellang requested a review from a team as a code owner May 3, 2024 13:11
@sburmanoctopus
Copy link
Contributor

Thank you for you submission.

We were wondering about the reasons behind doing this change. Could you please elaborate on the motivation?

@khellang
Copy link
Author

khellang commented May 9, 2024

Could you please elaborate on the motivation?

We encountered the error described in #605, where the server had the thumbprint in uppercase and the client had the thumbprint in lowercase (or vice versa).

@sburmanoctopus
Copy link
Contributor

Hi @khellang,

Thank you for taking the time to prepare this contribution, we appreciate and value your time, unfortunately this time we won't be able to accept this contribution.

Halibut is a very important part of Octopus Deploy, and technically this is a breaking change. While we agree this is a positive change, however the fact that consumers of the library should be able to account for it's shortcoming easily means we aren't able to justify the cost of making this breaking change right now.

@khellang
Copy link
Author

khellang commented May 17, 2024

Hi @sburmanoctopus ! 👋

Can you explain why this is a breaking change? I'm falling to see how this could break anything 😅

@sburmanoctopus
Copy link
Contributor

Hi @khellang,

To make this change we would also want to update the trust accepting side of halibut to be case insensitive, that way we don't have some checks as case sensitive and other checks as case insensitive. Users of Halibut can supply their own ITrustProvider, which won't know we have switched from case sensitive to case in-sensitive. In a sense that is a breaking change, although in theory it should never be a problem.

Since this is in the security area of Halibut, we would prefer to not make any change unless absolutely necessary. As we want to avoid inadvertently introducing a security vulnerability.

In theory a workaround to the issue your facing is to always upper case the thumbprint to the ServiceEndpoint`.

Since this issue has a workaround, and would require changes in the security areas of Halibut and with our general hesitation to add unnecessary changes to Halibut (we have Halibut services in very remote locations we don't want to break) let's see if holding off on this change is feasible.

@khellang khellang closed this May 21, 2024
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.

Certificate thumbprints should not be case-sensitive
2 participants