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.

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

[9.0] Include DiracX token in proxy PEM files #7261

Merged
merged 4 commits into from
Nov 27, 2023
Merged

[9.0] Include DiracX token in proxy PEM files #7261

merged 4 commits into from
Nov 27, 2023

Conversation

chrisburr
Copy link
Member

@chrisburr chrisburr commented Oct 30, 2023 •
edited
Loading

This pull request makes it so that a DiracX token is included in the PEM files (i.e. proxies) written by DIRAC. This makes it so that doing dirac-proxy-init, dirac-admin-get-proxy and similar all result in credentials being accessible by DIRAC for use with the legacy adaptor mechanism.

This is done by added the base64 encoded token to a -----BEGIN DIRACX-----/-----END DIRACX----- section in the PEM file. Software other than DIRAC completely ignores this, as required in the various RFCs that define the PEM format.

The main reason for suggesting this approach is so that setting X509_USER_PROXY and similar still functions in the people would expect without needing to define a convention with by which to associate the proxy to the DiracX token (which then gets messy once you consider containers, e.g. SinularityCE or LHCb's use of containers with lb-run).

Builds on top of #7258

Closes DIRACGrid/diracx#132

BEGINRELEASENOTES

*DiracX
NEW: DiracX credentials are now included in proxy PEM files created by DIRAC
NEW: DiracX is now mandatory 🎉

ENDRELEASENOTES

chaen
chaen reviewed Nov 1, 2023
View reviewed changes
src/DIRAC/Core/Tornado/Client/private/TornadoBaseClient.py Outdated
@@ -511,10 +511,12 @@ def _request(self, retry=0, outputFile=None, **kwargs):
# getting certificate
# Do we use the server certificate ?
if self.kwargs[self.KW_USE_CERTIFICATES]:
# TODO: Does this code path need to work with DiracX?
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably yes for agent without shifter proxy and webapp

Copy link
Contributor

Choose a reason for hiding this comment

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

So, remove this comment and following ones in this module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's more

Suggested change
# TODO: Does this code path need to work with DiracX?
# TODO: make this code path work with DiracX for Agents and possibly webapp ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We were leaning towards opening an issue rather than getting hung up on this right now

@chrisburr chrisburr mentioned this pull request Nov 2, 2023
Merged
@chrisburr chrisburr force-pushed the token-in-pem branch 6 times, most recently from ec316b2 to 34f5ac6 Compare November 27, 2023 13:14
@chaen chaen force-pushed the token-in-pem branch 2 times, most recently from 60c3d70 to dcf0f47 Compare November 27, 2023 13:47
@chrisburr chrisburr marked this pull request as ready for review November 27, 2023 13:57
@chrisburr chrisburr mentioned this pull request Nov 27, 2023
Merged
fstagni
fstagni reviewed Nov 27, 2023
View reviewed changes
src/DIRAC/Core/Tornado/Client/private/TornadoBaseClient.py Outdated
@@ -511,10 +511,12 @@ def _request(self, retry=0, outputFile=None, **kwargs):
# getting certificate
# Do we use the server certificate ?
if self.kwargs[self.KW_USE_CERTIFICATES]:
# TODO: Does this code path need to work with DiracX?
Copy link
Contributor

Choose a reason for hiding this comment

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

So, remove this comment and following ones in this module?

src/DIRAC/FrameworkSystem/scripts/dirac_proxy_info.py
Comment on lines +155 to +160
# Ensure the proxy is working with DiracX
try:
with DiracXClient() as api:
api.auth.userinfo()
except Exception as e:
invalidProxy(f"Failed to access DiracX: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

It this absolutely needed at this stage?

Copy link
Contributor

Choose a reason for hiding this comment

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

If what is absolutely need ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

tests/Jenkins/dirac_ci.sh Outdated
fi
pip install "${wheels[0]}"
else
pip install "git+https://github.com/DIRACGrid/diracx.git@main#egg=diracx-${wheel_name}&subdirectory=diracx-${wheel_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply

Suggested change
pip install "git+https://github.com/DIRACGrid/diracx.git@main#egg=diracx-${wheel_name}&subdirectory=diracx-${wheel_name}"
pip install diracx-core
pip install diracx-client

?

Copy link
Contributor

Choose a reason for hiding this comment

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

because we want to install from the main branch of the repository

setup.cfg
@@ -29,6 +29,8 @@ install_requires =
cachetools
certifi
diraccfg
diracx-client
diracx-core
Copy link
Contributor

Choose a reason for hiding this comment

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

in dirac_ci.sh you also added diracx-cli

Copy link
Contributor

Choose a reason for hiding this comment

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

This is for normal installation. In dirac_ci.sh, we install what's in the main branch

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only needed for the integration test setup itself

tests/Jenkins/dirac_ci.sh Outdated
source "${SERVERINSTALLDIR}/bashrc"

echo "==> Installing main branch of diracx"
for wheel_name in core client cli; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for wheel_name in core client cli; do
for wheel_name in core client; do

?

Copy link
Member Author

Choose a reason for hiding this comment

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

The CLI is needed for the CS sync that runs in this container. It's not ideal but it's good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

No we need the cli for the CS exporter cron

tests/Jenkins/utilities.sh Outdated Show resolved Hide resolved
src/DIRAC/Core/Security/DiracX.py Show resolved Hide resolved
@fstagni fstagni merged commit e240729 into DIRACGrid:integration Nov 27, 2023
1 check passed
@DIRACGridBot DIRACGridBot added the sweep:ignore Prevent sweeping from being ran for this PR label Nov 27, 2023
@chrisburr chrisburr deleted the token-in-pem branch November 28, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaen chaen chaen left review comments

@fstagni fstagni fstagni approved these changes

@atsareg atsareg Awaiting requested review from atsareg atsareg is a code owner

Assignees
No one assigned
Labels
sweep:ignore Prevent sweeping from being ran for this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bind X509_USER_PROXY to token location
4 participants
@chrisburr @fstagni @chaen @DIRACGridBot