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
RFC: privilege separation for ipa framework code #314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of reviews.
ipalib/install/certmonger.py
Outdated
| """ | ||
| Execute certmonger to request a server certificate. | ||
|
|
||
| ``dns`` | ||
| A sequence of DNS names to appear in SAN request extension. | ||
| """ | ||
| if storage == 'FILE': | ||
| certfile, keyfile = certpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API for file based certmonger requests is counter intuitive. I'd rather have an additional argument keyfile=None with a check if storage == 'FILE' and keyfile is None: raise ValueError('keyfile is required for file storage')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I do not understand what this refers to.
| @@ -1,7 +1,7 @@ | |||
| # Do not edit. Created by IPA installer. | |||
|
|
|||
| [Service] | |||
| Environment=KRB5CCNAME=$KRB5CC_HTTPD | |||
| Environment=GSS_USE_PROXY=yes | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the upgrade copy modified files like this one or ipa.service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read the PR decription, upgrade is not handled yet.
| @@ -0,0 +1,109 @@ | |||
| profileId=KDCs_PKINIT_Certs | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, why do we need a special Dogtag profile for the pkinit certs? I couldn't find an explanation on https://www.freeipa.org/page/V4/External_Authentication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the reason why we depend on pkinit is also in the PR description.
ipaplatform/base/paths.py
Outdated
| @@ -49,7 +49,7 @@ class BasePathNamespace(object): | |||
| HTTPD_IPA_CONF = "/etc/httpd/conf.d/ipa.conf" | |||
| HTTPD_NSS_CONF = "/etc/httpd/conf.d/nss.conf" | |||
| HTTPD_SSL_CONF = "/etc/httpd/conf.d/ssl.conf" | |||
| IPA_KEYTAB = "/etc/httpd/conf/ipa.keytab" | |||
| HTTP_KEYTAB = "/etc/gssproxy/http.keytab" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see code to remove and invalidate the old keytab /etc/httpd/conf/ipa.keytab. In case the principal should be disabled or invalidated, you can't useipa-rmkeytab. It does not invalidate the principal on the server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is none, once upgrade will be handle we'll mv and chown/chmod/chcon the file in the right place.
|
@simo5 TravisCI's pep8 checker is complaining about some PEP8 violations: |
|
Yeah going through those right now |
|
Updated branch, hopefully lint will be happy. |
|
@simo5 Please extend the design page with image description which explains each of the steps. There are numbers and letters in the image which are not explained anywhere. A detailed end-to-end example of interaction could be useful for detailed review. Thank you! |
|
Note: this PR also depends on and includes commits from #206 |
f27c461
to
ccd0f34
Compare
|
@pspacek I added workflows to the Design page, please verify |
6a0e382
to
8958770
Compare
a41dea5
to
898ed1f
Compare
|
I think this code is ready to be included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I have only reviewed the code (see inline comments), stay tuned for functional review.
freeipa.spec.in
Outdated
| @@ -255,6 +253,7 @@ Requires: systemd-python | |||
| Requires: %{etc_systemd_dir} | |||
| Requires: gzip | |||
| Requires: oddjob | |||
| Requires: gssproxy >= 0.5.1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a short comment about why 0.5.1 is the minimal required version (e.g. "0.5.1: https://ticket/url" or "0.5.1: has feature XYZ")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change to something like 0.5.2 in any case, do you want the comment in the spec file or in the commit message ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spec file please.
install/conf/ipa-gssproxy.conf
Outdated
| cred_store = ccache:FILE:/var/lib/gssproxy/http.ccache | ||
| impersonate = allow | ||
| cred_usage = both | ||
| euid = apache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The httpd UID is platform-specific. This file should be a template and the value of ipaplatform.constants.constants.HTTPD_USER should be used for the UID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
install/conf/ipa-gssproxy.conf
Outdated
| mechs = krb5 | ||
| cred_store = keytab:/etc/gssproxy/http.keytab | ||
| cred_store = client_keytab:/etc/gssproxy/http.keytab | ||
| cred_store = ccache:FILE:/var/lib/gssproxy/http.ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use an IPA-specific location (/etc/ipa/gssproxy, /var/lib/ipa/gssproxy) for the credentials? I think it would make things easier for IPA in containers, backup, etc. down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/var/lib/ipa/ would be probably best for containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
ipaplatform/base/constants.py
Outdated
| @@ -8,15 +8,19 @@ | |||
|
|
|||
|
|
|||
| class BaseConstantsNamespace(object): | |||
| ANON_USER = 'WELLKNOWN/ANONYMOUS' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a platform-specific constant, please move it to ipalib.constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
ipaserver/install/httpinstance.py
Outdated
| def request_anon_keytab(self): | ||
| parent = os.path.dirname(paths.ANON_KEYTAB) | ||
| if not os.path.exists(parent): | ||
| os.makedirs(parent, 0o755) # pylint: disable=old-octal-literal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the superfluous pylint comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was required to pass make pylint, will keep it until that stops complaining
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, this is not an old octal literal, so I don't see why it would complain about it.
| path = os.path.join(root, f) | ||
| os.chmod(path, 0o640) | ||
| os.chown(path, pent.pw_uid, pent.pw_gid) | ||
| tasks.restore_context(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please re-use HTTPInstance.create_cert_db() instead of duplicating its functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
| nickname='ipaCert') | ||
| certmonger.start_tracking(secdir=paths.IPA_RADB_DIR, | ||
| nickname='ipaCert', | ||
| password_file=paths.IPA_RADB_PWDFILE_TXT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates functionality of certificate_renewal_upgrade().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is about properly tracking the cert that has been moved from one place to another, certificate_reqnewal_upgrade() deals with renewing certs, but does not have conditional code to check if they moved, it would make things more complex and more obscure to do the re-tracking there instead of when and where the cert is being moved.
| pw = binascii.hexlify(os.urandom(10)) | ||
| p12file = os.path.join(paths.IPA_RADB_DIR, 'ipaCert.p12') | ||
| olddb.export_pkcs12('ipaCert', p12file, paths.ALIAS_PWDFILE_TXT, pw) | ||
| newdb.import_pkcs12(p12file, paths.IPA_RADB_PWDFILE_TXT, pw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would be preferrable to move this code to a function in ipaserver.install.server.upgrade and call it after creating the RA DB and before certificate_renewal_upgrade() is called, so that we can get rid of all of the duplicate code found in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed most upgrades should be done via plugins?
What's the rule ?
ipaserver/install/server/upgrade.py
Outdated
| want to clean up sysrestore.state to remove all references to | ||
| ipa_kpasswd. | ||
| """ | ||
| ipa_memcached = MemcachedInstance() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only use of MemcachedInstance and thus can be replaced with SimpleServiceInstance('ipa_memcached') and MemcachedInstance removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I just followed the same code that was used for the old kpasswd instance, should someone change that one too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
ipaserver/install/httpinstance.py
Outdated
| pwd_file = os.path.join(database, 'pwdfile.txt') | ||
| def create_cert_dbs(self): | ||
| self.create_cert_db(paths.HTTPD_ALIAS_DIR, constants.HTTPD_USER) | ||
| self.create_cert_db(paths.IPA_RADB_DIR, constants.IPAAPI_USER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think HTTPInstance has any bussiness in creating the RA DB, it should in fact be done in CAInstance.
Also it shouldn't be created in CA-less mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into whether moving it in the CA instance is feasible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this, don't we get a RA cert also on replicas that do not have the CA installed ?
In that case the CAInstance is not run ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I can give you a hand if you want.
|
|
Why is dogtag-ipa-renew-agent-submit part of the certmonger package ? |
|
You can specify the nickname using -n/--nickname. You'll probably also want to set --cafile=/etc/ipa/ca.crt, --dbdir=/etc/httpd/alias and sslpinfile=/etc/httpd/alias/pwdfile.txt to maintain current behavior. |
|
Rebased on master and fixed a couple minor lint issues |
|
@simo5, I might have fixed the certmonger issue, see HonzaCholasta@907ef3cff2045edd4625d4c422d1d0ae473fe51c, however I'm hitting the "No valid Negotiate header in server response" error again. Any idea what might be causing it? |
|
I switched all endpoints to use GSSAPI (and transparently use a session cookie once one transation is successful), so there may be some parts of the code a bit surprised about it, do you have apache logs to chare that show the problem ? (enabling ipa debug would probably help too) |
|
@simo5, I can't reproduce the bug anymore with the latest update. Pylint found one trivial issue: (It should be |
| try: | ||
| delattr(context, 'session_cookie') | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should delete context.session_cookie also in the except (errors.CCacheError, ValueError): branch of the try-except statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that later for ticket 6543 though.
|
@simo5, most of the commits do not have a ticket link, is this intentional? |
|
For some commits I was sure what ticket to use, for some I was not, so I elected not to put a specific ticket in there. If you have a good idea of what ticket (of the External Authentication project) to apply to specific commits let me know and I can amend commit messages. |
|
@simo5, is there an umbrella ticket? 5959 perhaps? |
|
I would personally go with:
|
Stop using memcache, use mod_auth_gssapi filesystem based ccaches. Remove custom session handling, use mod_auth_gssapi and mod_session to establish and keep a session cookie. Add loopback to mod_auth_gssapi to do form absed auth and pass back a valid session cookie. And now that we do not remove ccaches files to move them to the memcache, we can avoid the risk of pollutting the filesystem by keeping a common ccache file for all instances of the same user. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
We do not want to generate runtime directories just because the packages are installed, but only if the server is actually setup and run. Also this will be needed later because we will create a user at install time and some tmpfiles will need to be owned by this user. As we are changing this code also rationalize the directory structure and move it from the http rundir to the ipa specific rundir. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
The framework will not have direct access to the keytab anymore. This function was used in two places, to fetch the domain list and to re-initialize the PAC when enabling or disabling a domain trust. The domian list is normally fetched via oddjob anyway so this use is not necesary anymore, and the MS-PAC re-initialization can be moved later to oddjob if needed. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
The anonymous user allows the framework to obtain an armor ccache without relying on usable credentials, either via a keytab or a pkinit and public certificates. This will be needed once the HTTP keytab is moved away for privilege separation. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
https://fedorahosted.org/freeipa/ticket/4189 https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
This is in preparation for separating out the user under which the ipa api framework runs as. This commit also removes certs.NSS_DIR to avoid confusion and replaces it where appropriate with the correct NSS DB directory, either the old HTTPD_ALIAS_DIR ot the RA DB IPA_RADB_DIR. In some cases its use is removed altogether as it was simply not necessary. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
It seem like ALIAS_CACERT_ASC was just a redundant location for the CA cert file which is always available in /etc/ipa/ca.crt Just use the canonical CA cert location in /etc/ipa for all cases and stop creating a separate cacert file. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
Add the apache user the ipawebui group. Make the ccaches directory owned by the ipawebui group and make mod_auth_gssapi write the ccache files as r/w by the apache user and the ipawebui group. Fix tmpfiles creation ownership and permissions to allow the user to access ccaches files. The webui framework now works as a separate user than apache, so the certs used to access the dogtag instance need to be usable by this new user as well. Both apache and the webui user are in the ipawebui group, so use that. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
The RA database sould not be created by the HTTP instance, but in the code path that creates the CA instance. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
When uninstalling systemd is told to disable the service, but it is not told to sopt it, so it believes it is still running. This can cause issues in some cases if a reinstall is performed right after an uninstall, as systemd may decide to stop the disabled service while we are reinstalling, causing the new install to fail. https://fedorahosted.org/freeipa/ticket/5959 Signed-off-by: Simo Sorce <simo@redhat.com>
This allows code to use multiple ccaches without having to muck with the process global environment variables (KRB5CCNAME). https://fedorahosted.org/freeipa/ticket/6543 Signed-off-by: Simo Sorce <simo@redhat.com>
Instead of relying on side effects (setting the KRB5CCNAME env var), explicitly pass the ccache name to be used if it is not the default ccache. This fixes some tests that sometimes fail to work properly due to the wrong ccache being used. https://fedorahosted.org/freeipa/ticket/6543 Signed-off-by: Simo Sorce <simo@redhat.com>
If we are changing identiy (different principal) insure we remove the session cookie stored on the rpc context so that we do not mistakenly connect with the previous identity credentials. https://fedorahosted.org/freeipa/ticket/6543 Signed-off-by: Simo Sorce <simo@redhat.com>
|
Done |
|
Thank you. |
|
FYI, KRA and vault are broken because KRA cert is not migrated: https://fedorahosted.org/freeipa/ticket/6675 |
|
Cookie parsing bug with FreeIPA 4.4 client: |
|
I would put broken KRA cert migration to lowest priority since #367 moves the original KRA cert anyway. |
As part of the External Authentication work this PR implements the privilege separation portion of the design available here: https://www.freeipa.org/page/V4/External_Authentication and implements tickets: https://fedorahosted.org/freeipa/ticket/5959 and https://fedorahosted.org/freeipa/ticket/4189
The update process from an old server has not been implemented yet, so this is just an RFC request at this stage. Please look at the code and let me know if you notice any major issue with it so we can correct mistakes early.
This PR depends on improvements and fixes to two dependencies: mod_auth_gssapi and gssproxy, which are not released/accepted upstream yet (all PRs filed, and will be available soon).
In order to allow trying the code, I made two copr repos with the necessary changes available here:
I tested a new install and both gssapi as well as password authentication work (via command line and web browser). I have not tested OTP authentication yet.
There are 2 fundamental changes in this code:
This required two changes in the form-based authentication workflow:
@jcholast @pvoborni Please provide comments on the framework changes.
@rcritten @abbra do you have ideas on how to deal with dropping a service (memcached) on upgrade ?
EDIT: Until a SeLinux policy is developed, this code needs to be run in permissive mode or it will fail.