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

Fix OIDC access to verify TLS connection #3487

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

webbnh
Copy link
Member

@webbnh webbnh commented Jul 7, 2023

In the course of recovering from our latest challenges with the Staging server, we noticed warnings triggered by the Pbench Server accesses to the OIDC server indicating that TLS verification was disabled...which, in fact, it was...which is undesirable. It turns out that there are two places in the Pbench Server code where it connects to the OIDC server -- once during startup to make sure that the service is available and once somewhat later to fetch the public key for token decryption -- and the first was performing TLS verification but the second wasn't.

This PR provides a common mechanism for both sets of accesses to use to determine whether and how to use TLS verification. Both relevant places in the code now pull the path to the CA from the Pbench Server configuration; if the path is not provided, they use True to trigger default TLS verification using the system's trusted CA store.

This change also renames the configuration option and wordsmiths some of the relevant code comments.

@webbnh webbnh added Server Containerization Of and relating to the process of setting up and maintaining container images Functional Testing labels Jul 7, 2023
@webbnh webbnh added this to the v0.73 milestone Jul 7, 2023
@webbnh webbnh requested review from ndokos and dbutenhof July 7, 2023 19:28
@webbnh webbnh self-assigned this Jul 7, 2023
dbutenhof
dbutenhof previously approved these changes Jul 7, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Hmm. OK, fine. I don't see much benefit in the config option name change, and technically requests supports a directory of CA files (though we're unlikely to use it) making "location" perhaps slightly more accurate than "file". (Although "ca" is a bit more specific than "cert"...)

@dbutenhof
Copy link
Member

  • black --check .
    would reformat lib/pbench/server/auth/init.py

@webbnh
Copy link
Member Author

webbnh commented Jul 7, 2023

  • black --check .
    would reformat lib/pbench/server/auth/init.py

Thanks for the heads-up. Apparently I removed the trailing comma when I changed the line, which gave Black the latitude to rewrap it.... 🙄

@webbnh webbnh marked this pull request as ready for review July 7, 2023 20:20
@webbnh webbnh force-pushed the fix_keycloak_tls branch from d968c12 to da67f51 Compare July 7, 2023 20:20
@webbnh
Copy link
Member Author

webbnh commented Jul 7, 2023

Alright! This branch has been rebased on main and is ready for final review. (Apparently there are in fact no changes since the last round of reviews, because GitHub is still showing approvals...yay!) So, if the tests pass I guess I can merge with no further action, but I figured I would mention it anyway.

I don't see much benefit in the config option name change, and technically requests supports a directory of CA files (though we're unlikely to use it) making "location" perhaps slightly more accurate than "file". (Although "ca" is a bit more specific than "cert"...)

Yeah, I apologize for the churn. However, I didn't feel like the name was serving us well and I felt impelled to update the comments, so I figured I would try another name while I was at it. (Yes, the option is a bit of a swiss army knife, but I wanted "CA" in there, and so I followed our local usage which is "file".)

@webbnh webbnh merged commit 30ddc0b into distributed-system-analysis:main Jul 10, 2023
@webbnh webbnh deleted the fix_keycloak_tls branch July 10, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Containerization Of and relating to the process of setting up and maintaining container images Functional Testing Server
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants