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

PKI config template and preliminary HSM support #2307

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tiran
Copy link
Member

tiran commented Aug 30, 2018

This patchset changes CA and KRA installer to be more flexible and easier to understand. The CA and KRA installer no longer create an ini file completely from scratch.

  • common and hard-coded settings are in ipaca_default.ini
  • very few special cases and overrides are configured from a dict
  • ipaca_customize.ini contains settings that can be modified by users
  • ipa-server-install and other installers now take a path to an override ini.

The new approach allows users to customize several aspects of Dogtag without requiring us to add new arguments to all installers.

The patch set also addresses some aspects for HSMs and adds a simple test case for softhsm2.

@tiran tiran added the WIP label Aug 30, 2018

@tiran tiran force-pushed the tiran:pki_ini branch 7 times, most recently from 4dd0f2d to 2201294 Aug 30, 2018

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Aug 30, 2018

Is using a snapshot of the config a temporary thing? It seems like something that could bite later.

@tiran tiran force-pushed the tiran:pki_ini branch 21 times, most recently from 051749b to ec0e423 Aug 30, 2018

@freeipa-pr-ci freeipa-pr-ci removed the re-run label Jan 29, 2019

@tiran tiran force-pushed the tiran:pki_ini branch 2 times, most recently from c937abc to 359fa50 Jan 29, 2019

@tiran

This comment has been minimized.

Copy link
Member Author

tiran commented Jan 31, 2019

Test is currently broken, see https://pagure.io/dogtagpki/issue/3093

@frasertweedale frasertweedale self-assigned this Feb 4, 2019

@frasertweedale

This comment has been minimized.

Copy link
Contributor

frasertweedale commented Feb 5, 2019

@tiran nice work mate. I've read all your changes. Some comments inline (nothing major, and some bike-shedding I confess). I'll test tomorrow, and start investigating the sslserver using wrong token bug (https://pagure.io/dogtagpki/issue/3093), which you blame for the test failure.

@rcritten in relation to your comment, I would not worry about it. We usually preserve backwards compat when we make changes to installation knobs, for a few releases at least.

Show resolved Hide resolved install/tools/ipa-ca-install.in Outdated
Show resolved Hide resolved install/tools/ipa-ca-install.in Outdated

defaults = mangle_values(defaults)
subsystem_config = mangle_values(subsystem_config)
def _mangle_values(self, dct):

This comment has been minimized.

@frasertweedale

frasertweedale Feb 5, 2019

Contributor

Could you please make this a @staticmethod or move it out of the class?

This comment has been minimized.

@tiran

tiran Feb 5, 2019

Author Member

Python isn't Java. :) There is no point in using staticmethod at all. The decorator doesn't make the code faster or better. I'm aware that some IDEs nudge users to use staticmethod for internal methods that do not use self. I consider staticmethod as noise.

This comment has been minimized.

@frasertweedale

frasertweedale Feb 5, 2019

Contributor

I consider self as noise, if it is not used :) The decorator does improve readability by revealing, at a glance, that the code does not (and cannot) do certain things.

If you dislike the decorator you can promote the function to the module level, which achieves the same readability benefit.

Not a hill I'm going to die on. I don't mind if we go tit-for-tat with you firmly declining to use staticmethod and me subbornly refusing not to use it :D

This comment has been minimized.

@tiran

tiran Feb 6, 2019

Author Member

For me self is like the CMB radiation. It's always been there since the big bang of Python. Any time it's missing or deviates from the standard, something special is going on.

In Python @classmethod is a special case for alternative class constructors or similar methods that don't . @staticmethod shouldn't be used at all. Static methods were accidentally added, https://mail.python.org/pipermail/python-ideas/2012-May/014969.html

They're basically an accident -- back in the Python 2.2 days when I was inventing new-style classes and descriptors, I meant to implement class methods but at first I didn't understand them and accidentally implemented static methods first. Then it was too late to remove them and only provide class methods.

I have virtually never used staticmethod and I'm only using classmethod when it's absolutely required.

This comment has been minimized.

@frasertweedale

frasertweedale Feb 6, 2019

Contributor

There is a clear readability benefit to annotating that a method does not and cannot access instance variables (classmethod) or instance or class variables (staticmethod). Readability counts.

But we have been here before. As I said, it is not a show-stopper for me.

cfgtpl.read_file(f)

# sanity check
self._verify_immutable(cfgtpl, immutable_settings)

This comment has been minimized.

@frasertweedale

frasertweedale Feb 5, 2019

Contributor

I like this sanity checking very much.

For this first _verify_immutable, it would indicate developer error, right? (i.e. we have overridden something we should not have). In the second, optional case, it is because user-supplied override file overrode something it should not have.

So I think we should have different messages for these cases, even if the difference is as simple as:

"IPA PKI config overrides immutable options: ..."

versus

"User PKI config overrides immutable options: ..."

What do you think?

@@ -693,3 +771,30 @@ def mangle_values(d):
config.set(section_name, key, value)

return config


def test():

This comment has been minimized.

@frasertweedale

frasertweedale Feb 5, 2019

Contributor

Let's move this to the test suite.

@tiran tiran force-pushed the tiran:pki_ini branch from 359fa50 to cdd0db8 Feb 5, 2019

tiran and others added some commits Aug 30, 2018

Add current default.cfg from Dogtag
base/server/etc/default.cfg from commit
dogtagpki/pki@b931834

Signed-off-by: Christian Heimes <cheimes@redhat.com>
Simplify and slim down ipaca_default.ini
* Remove internal stuff from DEFAULT section
* Remove all non-user modifiable paths
* Remove OCSP, RA, TKS, TPS sections
* Remove deprecated options and replace them with current options

Signed-off-by: Christian Heimes <cheimes@redhat.com>
Add IPA specific vars to ipaca_default.ini
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Use new pki_ipaca.ini to spawn instances
Note: Some configuration stanzas are deprecated and have been replaced
with new stanzas, e.g. pki_cert_chain_path instead of
pki_external_ca_cert_chain_path.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
Add pki.ini overlay option
Allow to specify a pki.ini overlay file and token password on the
command line. The overlay file can be used to override pkispawn
settings.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
Implement basic HSM support
The HSM state is stored in fstore, so that CA and KRA installer use the
correct token names for internal certificates.

Key backup is disabled in HSM mode.

Token name is now used to identify the certificate and passed to certmonger.

Co-authored-by: Magnus K Karlsson <magnus-ka.karlsson@polisen.se>
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Split ipaca configuration into two files
ipaca_default.ini provides default values that can't be changed by an
overlay file. ipaca_customize.ini contains settings that can be
modified.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
Simplify and consolidate ipaca.ini
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Address inconsistent-return-statements
Pylint warns about inconsistent return statements when some paths of a
function return None implicitly. Make all implicit returns either
explicit or raise a proper exception.

See: https://pagure.io/freeipa/issue/7758
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Add workaround for p11-kit-proxy
Disable p11-kit-proxy integration for NSSDB so that Dogtag is able to
install SoftHSM2 PKCS#11 library by itself.

See: https://pagure.io/dogtagpki/issue/3091
Signed-off-by: Christian Heimes <cheimes@redhat.com>

@tiran tiran removed the needs rebase label Feb 5, 2019

@tiran tiran force-pushed the tiran:pki_ini branch from cdd0db8 to 6658f5a Feb 5, 2019

@frasertweedale

This comment has been minimized.

Copy link
Contributor

frasertweedale commented Feb 6, 2019

@tiran thanks for the updates. Should we also document the --pki-config-override option in the ipa-(server|ca|kra)-install man pages?

(edit) also, I wonder if it is possible in the --help output to include --pki-config-override in the Certificate system options section? (It is currently in Basic options).

(edit 2) also the --token-password option to be added to man pages.

@frasertweedale

This comment has been minimized.

Copy link
Contributor

frasertweedale commented Feb 6, 2019

@tiran another thing: the config immutable override checks should be performed during install_check(). As currently implemented, if you override an immutable config installation begins, then it fails.

'usermod', '-G', constants.ODS_GROUP, '-a', constants.PKI_USER
])
# change SELinux context from default named_cache_t to pki_tomcat_t to
# avoid AVCs for certutil and pkitool

This comment has been minimized.

@frasertweedale

frasertweedale Feb 6, 2019

Contributor

What are the implications of changing the SELinux context for DNS(SEC), which also uses softhsm (IIRC)?

This comment has been minimized.

@tiran

tiran Feb 6, 2019

Author Member

This is a hack to have some very basic level of testing. There is no proper solution yet. Both Dogtag and SoftHSM needs to improve to officially support SoftHSM with Dogtag.

IPA's DNSSEC integration uses a custom softhsm config file and a different storage location for the tokens.

def install(cls, mh):
cls.pki_ini, cls.token_pin_filename = prepare_softhsm(cls.master)
extra_args = [
'--pki-ini-overlay', cls.pki_ini,

This comment has been minimized.

@frasertweedale

frasertweedale Feb 13, 2019

Contributor

This needs to change to --pki-config-override.

"Invalid config '{}': {}".format(value, e)
)

token_password = knob(

This comment has been minimized.

@frasertweedale

frasertweedale Feb 13, 2019

Contributor

We most likely want a --token-password-file option, instead of or in addition to --token-password, so that it is possible to convey the password with being visible in /proc and similar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.