Need to replace spaces with a tab to retain style.

Would be nice to have this broken out as a separate commit.

I assume this is the C-99 reference? Also should be a separate commit.

Please remove this.

I have a somewhat similar patch in my development branch but I think I prefer some of the decisions you made. I have mostly nit comments so far. I'll see if I have any suggestions based on what I've done.

The base I started from was a patch adding support for the SCEPStandard capability which sets the cipher to SHA-256 and the digest to AES-128.

@rcritten Pushed an update that should address the comments.

copy-paste issue, list is the cipher list.

nit: maybe it's personal preference but I don't really like the term authoritatively here. It is probably clearer to say it was set from configuration.

nit: can you specify the version of the draft? It seems like the spec is still moving fast.

I'm not sure how this config-path related to the rest of the patch? Is this part of another, unrelated change?

@rcritten config-path was added because I was having a heck of a time trying to figure out where my configuration was getting dropped. This let me copy/paste it while trying to determine what file I needed to set the ciphers in.

@rcritten Are there any other issues that need to be addressed before I update this?

It was pretty close last I looked. Be sure you run the unit tests as well (make check).

I'll need to try this in a few configurations but in general I think its nearly done.

@rcritten How do I test dynamic data in the expected.out files in the tests?

The main issue that I needed to solve was figuring out what CA was in what directory and, of course, those are date stamped (which was the issue) so the output changes based on the time of the test run.

@rcritten I figured out a way to make the tests work appropriately. It's not...great...but it's all that I could come up with without making large numbers of changes. PR coming after the tests all pass.

@rcritten Code and tests should all be up to date now.

Ok, sorry for not getting back to you sooner. I"ll take a look.

Thanks for not squashing in the feedback changes, that made it much easier to re-review :-)

Your run-tests change is clever :-)

I see this on output: "Files /tmp/runtests1pK0H6 and expected.out differ". I think i'd be ok adding ">/dev/null" to suppress the output as it seems confusing to have a difference but the test pass. I'll accept it either way, just curious what you think.

The other option would have been to use sed to replace the id with some fixed value but diff seems more straightforward.

Code looks good, just a few git nits:

Can you add "https://pagure.io/certmonger/issue/89" to the SCEP and test commits? This will help tie things back to the original history when doing git archaeology.

nit in commit #3 commit message: the default digest was changed to SHA-256.

Can you add a more descriptive message on the test patch (5), including your diff -I workaround for ignoring the mismatching lines?

I suppose I'd prefer separate PRs for the first two patches but I'm not going to make a federal case out of it. Just something to remember for next time (these could have been pushed separately long ago).

Next step is to fix up the commit changes, go ahead and squash the feedback commit into the SCEP commit and this is good to go.

Found one more very minor thing. For both digest and cipher if GetCACaps fails it will say:

Could not determine supported CA capabilities, using

Can you add in digest/cipher there to be totally clear? It's fairly obvious if you know the spec but for a newbie...

I think the new options need to be documented somewhere as well I'm just sure where yet. It is non-obvious to stop certmonger, update the config (the -v is super-helpful, you're right) and then restart.

I'm planning on doing a certmonger release next week so let me know if you think you'll get to this Monday or Tuesday and I can hold off a little.

BTW I recently pushed a patch to master that causes helpers to log to syslog when stderr is closed so this should improve debugging capabilities (one or more -v is still required to get much out of it).

@rcritten Yeah, I'll fix the rest of the items by Monday.

@rcritten Just pushed the update. It's..sort of like Monday

Great work, thanks for your contribution.