@abbra @frenaud I realize you are most likely unfamiliar with certmonger code but if you have time I'd appreciate a second set of eyes on this.

I made a build with these changes and the child KILL patch at https://copr.fedorainfracloud.org/coprs/rcritten/certmonger/

Instead of strcpy() and snprintf() below, please use krb5_build_principal_ext() or krb5_build_principal() from libkrb5. They differ in that one accepts explicit length and char-data pairs and the other one accepts null-terminated strings.

Actually, krb5_get_init_creds_keytab works just fine with tgs being NULL, that would be an equivalent of what you are doing with tgs above. It means that the whole code that generates tgs can be removed and tgs in this call can be replaced with NULL.

The use of talloc structures here is backwards to what it was designed for.
Please create a temporary context before doing a referrer thing above. Then allocate the referrer off that context and supply the context to cm_submit_h_init().
Finally, you'd only need to clean up the context, not individual elements, so no need to do talloc_free(referer); or talloc_free(hctx);.

One more thing is that you pass referer as 7th argument to cm_submit_h_init() and twice passing application/json to content_type and accept. However, 7th argument is cainfo and there is also cainfo passed in. Looks like it is an error?

(A comment around cm_submit_h_init() signature should be ignored, it is due to how pagure displays the diff.

Hi @rcritten

• After an upgrade to your copr version, the CA helper "certmaster" is still displayed with getcert list-cas. A %triggerin scriptlet may be added to certmonger.spec in order to delete the CA helper definition from /var/lib/certmonger/cas

• if I call getcert resubmit -i with one of the IPA certs tracked by the IPA helper, I get an error in /var/log/httpd/error_log:

ipa: INFO: [jsonserver_kerb] host/master.domain.com@DOMAIN.COM: cert_request(None, principal='krbtgt/DOMAIN.COM@DOMAIN.COM', add=True): OptionError
ipa: INFO: exception OptionError caught when converting options: Unknown option: profile

and the journal displays:

certmonger[381314]: 2020-08-26 14:06:39 [381314] Running enrollment helper "/usr/libexec/certmonger/ipa-server-guard".
ipa-submit[381315]: JSON-RPC error: 3005: Unknown option: profile
ipa-submit[381315]: JSON-RPC error: 911: Missing or invalid HTTP Referer, Referer: https://master.domain.com/ipa/json

Dropped the tgs part.

Generate a talloc context and pass that to cm_submit_h_init().

The argument ordering looks ok to me.

I fixed the profile option (to profile_id)

And remove the certmaster CA in %post if xmlrpc isn't enabled.

I've submitted another copr build with the related fixes included.

Hi @rcritten
Tested with your copr repo:

• the upgrade now properly removes certmaster CA helper
• could not reproduce any more the renewal lock issue
• getcert resubmit -i <id> now passes profile_id, the renewal is working

I will let @abbra review the code in depth but the PR is working as expected.

@ftweedal you have some experience with certmonger. If you have a chance, can you take a peek?

@rcritten sure, I will review it tomorrow (wednesday).

This doesn't look right (note the extra '+' in front of endif).

A couple of fixes are needed to get the srpm generation working:

diff --git a/certmonger.spec b/certmonger.spec
index 9d8f2ccb..2ccad7e6 100644
--- a/certmonger.spec
+++ b/certmonger.spec
@@ -172,7 +172,7 @@ fi
 if test $1 -gt 1 ; then
     %{_bindir}/getcert remove-ca -c certmaster 2>&1 || :
 fi
-+%endif
+%endif
 %if %{systemd}
 if test $1 -eq 1 ; then
        /bin/systemctl daemon-reload >/dev/null 2>&1 || :
diff --git a/configure.ac b/configure.ac
index 14991244..f2964856 100644
--- a/configure.ac
+++ b/configure.ac
@@ -876,6 +876,7 @@ else
        AM_CONDITIONAL(HAVE_EC,false)
        AM_CONDITIONAL(WITH_IPA,false)
        AM_CONDITIONAL(WITH_CERTMASTER,false)
+       AM_CONDITIONAL(WITH_XMLRPC,false)
        AM_CONDITIONAL(WITH_LOCAL,false)
        AM_CONDITIONAL(HAVE_UUID,false)
 fi

Also, an older commit (removal of DSA tests) broke some stuff. I made a separate PR to fix that: https://pagure.io/certmonger/pull-request/169 .

And the produced SRPM fails to build - haven't yet investigated but here's the COPR build: https://copr.fedorainfracloud.org/coprs/ftweedal/certmonger-jsonrpc/build/1663049/ .

It failed due to a missing BuildRequires: krb5-devel

Clearly I don't normally test the srpm target. I think I have it worked out. The current target pulls a fresh pull from upstream and not from the local directory to actually test out changes. I'll add a new target.

I also forgot to disable certmaster in the tests (it is included in the CA counts, for example).

I may just kill certmaster altogether in the tests and not try to do some acrobatics to support with and without, we'll see.

I merged the dangling test targets patch, thanks.

I killed certmaster in the 028-dbus test so make check will pass.

I added a patch that adds a new target, local-srpm, to build from the current tree rather than from master. I took the output from 'make local-srpm' and ran it through mock and it built successfully.

@rcritten it's not for this PR, but the way SRPMs are built is yuck. Like... clone the origin, then clone it again, then do the stuff. It can be cleaned up a lot. I cannot think of any use case for the current srpm target to clone from origin.

Would also be nice to have a check for dirty working tree and abort with warning - based on how the srpm gets built, that's usually a mistake. I've been bitten several times making the SRPM, building, installing, testing, then realising the changes I wanted to test were not even included >_<

So I think: git diff --quiet to ensure no modified files, then git read-tree followed by git-checkout-index to a temp dir. (You can override the git index file and working dir with environment variables; see https://github.com/frasertweedale/dotfiles/blob/master/.gitconfig#L91-L103 for example).

@rcritten build works for me now. I installed IPA and did a getcert resubmit on the HTTP cert. I haven't done thorough testing, but I see in the diff that profile_id and cacn (issuer identifier) are being passed. Looks good from my POV.

Re: the srpm target yeah, I never understood its purpose which is why I hadn't tested with it. In the end it did prove to be useful since it discovered a typo and missing BR in the sample spec file.

I'm honestly not sure how much effort i want to put into supporting it.

I think this works well enough to merge into master and do additional live testing in Fedora via the FreeIPA CI system.