Skip to content

Made ODH cert default cert for Token Auth #489

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Made ODH cert default cert for Token Auth #489

wants to merge 3 commits into from

Conversation

Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Mar 22, 2024 •
edited
Loading

Issue link

RHOAIENG-52, RHOAIENG-4375

What changes have been made

For TokenAuthentication the SDK will use the cert injected into a ODH/RHOAI Notebook by default in the /etc/pki/tls/custom-certs/ca-bundle.crt location

Verification steps

Setup

Required steps for ODH/RHOAI

  • ODH > 2.9
  • Create a Data Science Workbench
  • Ensure that the path /etc/pki/tls/custom-certs/ca-bundle.crt exists in the notebook.

Notebook server ODH

  • Clone this repository with git clone https://github.com/project-codeflare/codeflare-sdk.git
  • Checkout this PR's branch
  • Run poetry build - install if needed (pip install poetry)
  • Run pip install --force-reinstall dist/codeflare_sdk-0.0.0.dev0-py3-none-any.whl
  • Restart your notebook kernel

Testing

Authenticate with TokenAuthentication.
Set the environment variable CA_CERT_PATH to the path to your cert before authenticating

auth = TokenAuthentication(
    token = "XXXXX",
    server = "XXXXX",
    skip_tls=False
)
auth.login()

You should receive the message Authenticated with certificate located at /etc/pki/tls/custom-certs/ca-bundle.crt
You should be able to run through any demo notebook.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from bobbins228. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dimakis
Copy link
Contributor

dimakis commented Apr 4, 2024

Whats the status of this PR? it looks like the PR with which it depends (kubeflow) on has been merged.
we need this for disconnected correct?

@Bobbins228
Copy link
Contributor Author

Whats the status of this PR? it looks like the PR with which it depends (kubeflow) on has been merged.
we need this for disconnected correct?

Kubeflow PR is in this is good to merge after a review and yeah pretty sure it's needed for disconnected

KPostOffice
KPostOffice reviewed May 10, 2024
View reviewed changes
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Nice changes, should make things much smoother!

src/codeflare_sdk/cluster/auth.py Outdated
@@ -101,12 +101,16 @@ def login(self) -> str:
"""
global config_path
global api_client
odh_ca_path = "/etc/pki/tls/custom-certs/ca-bundle.crt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this into __init__ and set self.ca_cert_path directly? and also allow the default path to be configurable via an environment variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are some great suggestions I'll make those changes

KPostOffice
KPostOffice reviewed May 14, 2024
View reviewed changes
src/codeflare_sdk/cluster/auth.py Outdated
@@ -106,10 +106,24 @@ def login(self) -> str:
configuration.api_key_prefix["authorization"] = "Bearer"
configuration.host = self.server
configuration.api_key["authorization"] = self.token
if self.skip_tls == False and self.ca_cert_path == None:
ca_path_env = os.environ.get("CA_CERT_PATH")
Copy link
Collaborator

Choose a reason for hiding this comment

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

os.environ.get("CF_SDK_CA_CERT_PATH", ca_path_env)

os.environ.get allows defaulting which can be used here. We should also prefix env vars WDYT of CF_SDK_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. I should also update the documentation in this PR too

@KPostOffice KPostOffice mentioned this pull request May 21, 2024
Merged
4 tasks
@KPostOffice
Copy link
Collaborator

see #544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KPostOffice KPostOffice KPostOffice left review comments

@anishasthana anishasthana Awaiting requested review from anishasthana

@astefanutti astefanutti Awaiting requested review from astefanutti

@ChristianZaccaria ChristianZaccaria Awaiting requested review from ChristianZaccaria

@Fiona-Waters Fiona-Waters Awaiting requested review from Fiona-Waters

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bobbins228 @dimakis @KPostOffice