Add MFA support for SSHExecutor #348
Conversation
giffels
force-pushed
the
add/mfa-ssh-support
branch
from
af3ebc4
to
1de0bee
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #348 +/- ##
==========================================
+ Coverage 98.92% 98.93% +0.01%
==========================================
Files 55 55
Lines 2226 2255 +29
==========================================
+ Hits 2202 2231 +29
Misses 24 24 ☔ View full report in Codecov by Sentry. |
| @@ -75,7 +75,7 @@ | |||
| # | |||
| # This is also used if you do content translation via gettext catalogs. | |||
| # Usually you set "language" from the command line for these cases. | |||
| language = None | |||
| language = "en" | |||
There was a problem hiding this comment.
Unrelated change. sphix was complaining about language is None.
giffels
force-pushed
the
add/mfa-ssh-support
branch
from
39f6196
to
121d8d8
Compare
giffels
force-pushed
the
add/mfa-ssh-support
branch
from
121d8d8
to
ce68e20
Compare
| new_cls = cls | ||
| if isinstance(node, yaml.nodes.MappingNode): | ||
| parameters = loader.construct_mapping(node) | ||
| parameters = loader.construct_mapping(node, deep=settings.eager) |
There was a problem hiding this comment.
This is necessary since MFA is using nested lists and dictionaries, when initialised using a yaml tag. This function is only used in the unittest. In real life it will be evaluated by COBalD, which already supports eager evaluation of yaml tags.
giffels
requested review from
maxfischer2781,
a team and
RHofsaess
and removed request for
a team
docs/source/executors/executors.rst
Outdated
| mfa_secrets: | ||
| - prompt: "Enter 2FA Token:" | ||
| secret: "IMIZDDO2I45ZSTR6XDGFSPFDUY" |
There was a problem hiding this comment.
Sorry, my head is a bit hung op on having mfa_secrets->secret. It duplicates the information without saying much.
- Instead of
mfa_secretsI recommend to usetotpso it's clear which method we are dealing with (not that I necessarily expect another, but it's free lunch). - Instead of
secretI could also seekeyworking here, since that is what it's called often. However,secrethas the added benefit of being just as long aspromptand I kind of like how the colons align... 🫣
There was a problem hiding this comment.
What about calling it mfa_config instead?
mfa_config:
- prompt: "Enter 2 FA TOken:"
secret: "IMIZDDO2I45ZSTR6XDGFSPFDUY"Three colons aligned. 👍
There was a problem hiding this comment.
However, I think
mfa_config:
- prompt: "Enter 2 FA TOken:"
totp: "IMIZDDO2I45ZSTR6XDGFSPFDUY"is much better. Since it's clear that we are just supporting totp.
Co-authored-by: Max Kühn <maxfischer2781@gmail.com>
This pull request adds support for multi-factor authentication (MFA) to the
SSHExecutorto be used with clusters enforcing MFA to access login nodes for job submission.