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

Krb5: handle [plugins] subsection #663

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

ptoscano
Copy link
Contributor

No description provided.

@ptoscano
Copy link
Contributor Author

ping?

@jcpunk
Copy link
Contributor

jcpunk commented Jun 15, 2020

A quick test on the Fermi config has this working for some of our plugin plans. I'd love to see this merged.

{ "disable" = "k5identity" }
}
{ "pwqual"
{ "module" = "mymodule:/path/to/mymodule.so" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be easier to work with as a deeper tree where module is a seq with mymodule = /path/to/mymodule.so so folks who need to easily switch from /usr/lib to /usr/lib64 for a specific module can target more directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how seq would help in the case you mentioned, since you can use the following expressions to find your node:

  • /files/etc/krb5.conf/plugins/*/module[. =~ regexp(".*/mymodule2.*")]
  • /files/etc/krb5.conf/plugins/*/module[. =~ regexp("mymodule:.*")]

Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid seq as much as possible when it's not necessary to use it. Imo seq is only useful when several consecutive nodes have the same label and you want to make sure they don't exchange indent/spaces when one of them is removed.

module = mypreauth:/path/to/mypreauth.so
}
ccselect = {
disable = k5identity
Copy link
Contributor

Choose a reason for hiding this comment

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

disable and enable_only might need to be a seq as it can be specified multiple times https://web.mit.edu/kerberos/krb5-1.18/doc/admin/host_config.html

@ptoscano
Copy link
Contributor Author

@jcpunk (replying here about both your notes wrt seq)
I thought about using seq intially, then I had the following thoughts:

  • other parts of krb5.conf can have repeated keys, and there is no seq used; thus using seq here would make it a bit incoherent with the existing layout of the file
  • using seq would mean having "normal" keys and seq keys mixing together, which IMHO is a bit ugly
  • if any existing key will be allowed to be repeated in the future, then it will not have a seq subtree (and doing so would mean breaking the compatibility of the generated tree, which I don't think it is a good idea)

@jcpunk
Copy link
Contributor

jcpunk commented Jun 16, 2020

Good call, I think all that makes sense.

Copy link
Member

@raphink raphink left a comment

Choose a reason for hiding this comment

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

LGTM

@raphink raphink merged commit fb87bf9 into hercules-team:master Aug 31, 2020
@ptoscano ptoscano deleted the krb5-plugins branch August 31, 2020 09:33
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.

3 participants