Include challenge password attribute if required by EST server #38

mobe1 · 2024-03-15T09:46:55Z

The changes introduced come after the issue/feature requrest #30 has been opened.
They allow us to enroll a CSR that includes the TLS-unique value as recommended by the RFC 7030

Because each http client instantiation results in a new TLS-unique, one way of including it would be to make EST requests from the same http client.
Because the standard crypto/x509 Go package does not handle the challenge password attribute (OID) the way an EST/CA server expects it, the CSR creation had to be wrapped.

Code refactoring : change Enroll(), Reenroll() and ServerKeyGen() csr argument type so that we don't depend on x509 package anymore, which today still ignores the challenge password attribute Inlcude tls unique if required by CA Add test cases Add sample Update readme and documentation

toddgaunt-gs · 2024-03-15T12:58:58Z

Thanks for opening this PR, I'll forward this to my team for review.

toddgaunt-gs · 2024-04-09T15:23:16Z

[ ] Need to resolve conflicts after upgrading to Go 1.22.1...

…tlsunique

mobe1 · 2024-11-12T13:37:39Z

Thought the comment wasn't for me until I noticed the repo did get upgraded with a different version of go...
The conflicts should be resolved.

toddgaunt-gs · 2024-11-12T13:47:14Z

Ah it wasn't actually! Just a reminder for when I had time to get around to this again, but thank you for addressing the changes needed!

DDvO · 2025-04-04T08:58:20Z

Looks like work on this PR has stalled - I wonder when is it going to be resumed?

DDvO · 2025-04-04T08:58:51Z

BTW, copying to csr.go large chunks of code from the x509 package does not look right -
there should be a lean way of adding and reading the challengePassword attribute of a PKCS#10 CSR.

toddgaunt-gs · 2025-04-09T12:55:57Z

Hi DDvO, unfortunately my team hasn't been able to spare the cycles to work on this repository for a while, so while I hesitate to recommend it you may want to fork and patch a clone of this repository if you need this functionality right away.

Edit: I'll try to review this PR as soon as possible to avoid such scenario

toddgaunt-gs · 2025-04-09T13:06:33Z

BTW, copying to csr.go large chunks of code from the x509 package does not look right - there should be a lean way of adding and reading the challengePassword attribute of a PKCS#10 CSR.

I agree with this, I'd prefer to see this functionality without including this dependency if possible.

toddgaunt-gs · 2025-04-09T13:18:31Z

Sorry for such a long delay for reviewing this PR, I didn't prioritize it and it has been far to long so I apologize for that. Thank you for your contribution, but I don't think I can hit the merge button on this PR as it is now into the master, but I am happy to support it as a separate branch to support this use-case for now.

Some critiques that are blocking this:

I'd prefer the smaller style changes, such as using io over ioutil and changing the code to call EqualFold be done in a separate PR as these changes are unrelated to the challenge password.
As @DDvO mentions above, avoiding copying csr.go would be preferred
The samples and resources directory added I'll have to give some more thought to. While I like seeing a way to try out the changes, I think I'd prefer this just as an automated test of the feature instead. I think adding the case to est_test.go would be the way to go.

mobe1 · 2025-04-10T07:53:41Z

Hi guys,

Thank you @DDvO for closing the previous PR, that's on me.

BTW, copying to csr.go large chunks of code from the x509 package does not look right -
there should be a lean way of adding and reading the challengePassword attribute of a PKCS#10 CSR.

I too agree, I actually borrowed that code snippet from another repository, credit goes to micromdm.

Meanwhile, unfortunately Go has still not taken action to address it, there is an open issue for it already.

mobe1 · 2025-04-10T08:43:27Z

@toddgaunt-gs
Nah acutally, you're right. I shouldn't have mixed code refactoring and the feature itself as it doesn't help telling what's necessary to make the feature available...

On a personal note: I thought I had to change the method signature but after a quick review, I start to think it can be done while keeping your methods signatures intact.

That being said,

I'll revert those unrelated changes
then I'll bring the method signature back
finally, i'll see if there is a better way of adding that challengePassword attribute without copying the csr.go (spoiler alert, i'm not very confident on this one)

DDvO · 2025-04-10T13:05:26Z

Hi guys,

thank you for swiftly responding on this.

BTW, copying to csr.go large chunks of code from the x509 package does not look right -
there should be a lean way of adding and reading the challengePassword attribute of a PKCS#10 CSR.

I too agree, I actually borrowed that code snippet from another repository, credit goes to micromdm.

Looks like there is a glitch in the link you included and instead you meant
https://github.com/micromdm/scep/blob/main/cryptoutil/x509util/x509util.go

Meanwhile, unfortunately Go has still not taken action to address it, there is an open issue for it already.

I've meanwhile had a closer look.
Oh my goodness. The crypto/x509 package screwed up the implementation of the PKCS#10 CSR Attribute type.
This bug is known since long and unfortunately is hard to work around also due to its very restrictive API.
I just commented there accordingly.

* add challenge password to CSR as described in rfc 7030 section 3.5 * update cmd client test : if challenge password is to be included, CSR must be re-created. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Rob Casey <rcasey@gmail.com> Co-authored-by: Todd Gaunt <todd.gaunt@globalsign.com> Co-authored-by: Rob Casey <61131@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…tlsunique

…sunique' into features/include_tlsunique

…tlsunique

mobe1 · 2025-07-15T10:18:43Z

Hey guys,

It appears Go hasn't moved much since, aside from labeling it NeedsInvestigation.
@DDvO, I checked out the CSR tool project your referenced as well

it does add the challenge password to the CSR
however, that's the only thing it does (most of the CSR fields are still missing)

Also noticed the project hasn't moved since its creation date and the fact he did not respond to your remark aren't very good signs.

At this point, I don't know if there are better alternatives

I took the shortcut of using existing code from micromdm scep package that already "solved" the problem, which implied a huge code duplication from x509 package though
Wait for Go to act on it and fix the CSR attribute implementation

mobe1 · 2025-07-15T10:27:38Z

As for the current PR, i don't know how ok you'd be with code duplication related to adding the challenge password in the CSR (i understand).

Of course, it's better for us if it gets merged as we would just use the package as is and wouldn't have to maintain a fork or anything.

No rush - let me know when you can.

toddgaunt-gs · 2025-07-15T12:10:27Z

Hey @mobe1, I think given the current situation with the upstream investigation not making much progress, some code duplication would be fine. I'll try to review this again sometime this week, if not today.

toddgaunt-gs

Unfortunately after further review I still don't think I'd like to merge this PR as is. Thank you for addressing prior feedback, but the following changes would need to be made before I merge this PR into the master branch.:

Review comments addressed
The sample program removed or rewritten into a directory containing a config file for the EST client and either a shell script or instructions for how to operate it for the use case it is demonstrating
The copied challenge password code found in parts of csr.go isolated into its own package with a clear README file indicating the source without any new or custom code included alongside it.

I understand that these changes I'm asking for may not be a priority for you as you have a functional fork of this repository working for you, and updates to this one are few and far between. Hopefully this review can at least benefit your own fork if you do decide you want to pursue these changes. Let me know if so and we can discuss in more detail each point to avoid misunderstanding any of the requested changes.

client.go

cmd/sample/utils.go

csr.go

mobe1

Yes, i did miss a critical detail on PoP in the context of mTLS. I'll get back to it as soon as i can.
More details on the corresponding comment.

As for the /sample directory, I'll just drop it cause there is no straightforward way to show it added the challengePassword.
I used to verify it via the EST server. It was easy and quick for me to just go run it.

client.go

…s included

…pasting

mobe1 · 2025-09-22T08:47:54Z

Hi @toddgaunt-gs, how you doing :)

Sorry for the delay, I had some time off and realized I haven't updated my PR since.

I closed the obvious comments :

I directly import the package that includes the tls-unique/challenge password (instead of copy/pasting with a comment).
The sample directoy was removed as it doesn't provide any value to the repo itself.

Finally, I fixed the defect I was about to introduce, I left the comment open for reminding the context and the agreed solution.
I also updated the EST client CLI along the readme.

Let me know if something else needs to be done.

mobe1

Noticed a few changes that are not longer required considering the latest solution that has been suggested/accepted.

I'll deal with them right away.

README.md

est_test.go

mobe1 · 2025-10-07T12:54:42Z

@toddgaunt-gs hi,
I think i have reverted all of the unrelated changes.

I saw there hasn't been a tag yet in one of your packages.
=> I used the commit hash instead in the meantime.

And, i'm not sure if it's relevant, but just in case, I made sure I didn't break anything in the CLI.
I used different options and checked the csr signing-key did not interfere with the mTLS private key.

I tested with a different CA to verify the final CSR the estclient CLI sent did include the challenge password.

Tried with your testing server .exe as well, and it works just fine.
It's just I don't have a direct way of checking whether it includes any challenge password or not.

toddgaunt-gs · 2025-10-16T12:24:29Z

Thanks for pinging me, sorry I'd been busy and haven't gotten back around to this. I'll take a look today.

client.go

go.sum

toddgaunt-gs · 2025-10-16T13:09:31Z

LGTM outside of a few small things. Feel free to address them if you have the time, though they aren't critical. I'll request another coworker to review in the meanwhile.

mobe1

Welcome back.

client.go

go.sum

mobe1 · 2025-10-22T16:31:33Z

Hi @mara-soldan welcome :)

I don't know when you'll get the time to review the PR.
But here's a little briefing of what has been done if it helps

I took advantage of the existing makeHTTPClient to get the tls-unique session/value (aka, challenge password in EST specs)
I imported the smallstep SCEP package which handles the challengePassword attribute in a CSR
- it basically wraps the standard x509.CertificateRequest which does not handle it correctly
- once the issue is resolved in the framework Go, we can then remove the dependency to smallstep custom function
And I updated Globalsign EST client CLI as well

And of course, if you see something else, let me know and I'll address it as soon as I can.

toddgaunt-gs · 2025-11-10T13:29:43Z

@mobe1 merged since I didn't want to hold this up needlessly. Thanks for the contribution, and for being patient and pleasant to work with during the review/feedback cycle!

mobe1 · 2025-11-12T13:30:17Z

@toddgaunt-gs I can say the same.
Appreciate !

toddgaunt-gs requested review from eliot-gs, tembio and toddgaunt-gs April 30, 2024 14:16

Merge remote-tracking branch 'upstream/master' into features/include_…

5895be8

…tlsunique

DDvO mentioned this pull request Apr 4, 2025

Inlcude challenge password attribute if required by EST server #34

Closed

DDvO mentioned this pull request Apr 10, 2025

crypto/x509: CertificateRequest does not support attributes not covered by pkix.AttributeTypeAndValueSET golang/go#15995

Open

mobe1 and others added 5 commits April 18, 2025 17:28

Merge remote-tracking branch 'upstream/master' into features/include_…

f044ea8

…tlsunique

Merge remote-tracking branch 'refs/remotes/origin/features/include_tl…

affaf1f

…sunique' into features/include_tlsunique

Merge remote-tracking branch 'upstream/master' into features/include_…

385e36c

…tlsunique

revert unrelated change to the challenge password feature

d736acd

toddgaunt-gs requested changes Jul 15, 2025

View reviewed changes

client.go Outdated Show resolved Hide resolved

cmd/sample/utils.go Outdated Show resolved Hide resolved

csr.go Outdated Show resolved Hide resolved

mobe1 commented Jul 16, 2025

View reviewed changes

client.go Outdated Show resolved Hide resolved

client.go Outdated Show resolved Hide resolved

Remove the sample from the repository

1c5365f

mobe1 added 7 commits August 5, 2025 10:40

remove .md warnings related to the syntax

bb8b02e

introduce signing key field for signing CSR when challenge password i…

dd26f9e

…s included

take into account signing key in est client CLI

bf8db4d

use smallstep scep package to add challenge password instead of copy …

deafdeb

…pasting

readme : enforce pop as per RFC 7030

8791396

go mod update

7ea2f21

Merge branch 'master' into features/include_tlsunique

1894829

mobe1 commented Oct 7, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

est_test.go Outdated Show resolved Hide resolved

est_test.go Outdated Show resolved Hide resolved

mobe1 added 2 commits October 7, 2025 13:21

revert remaining unrelated changes

da9c251

godoc update

14ec4d9

mobe1 requested a review from toddgaunt-gs October 16, 2025 12:14

toddgaunt-gs previously approved these changes Oct 16, 2025

View reviewed changes

client.go Outdated Show resolved Hide resolved

go.sum Show resolved Hide resolved

mobe1 commented Oct 16, 2025

View reviewed changes

client.go Outdated Show resolved Hide resolved

go.sum Show resolved Hide resolved

return error early when parsing certificate to prevent go from panic

dae7d89

mobe1 dismissed toddgaunt-gs’s stale review via dae7d89 October 16, 2025 13:22

mobe1 requested a review from toddgaunt-gs October 16, 2025 13:33

toddgaunt-gs approved these changes Oct 16, 2025

View reviewed changes

toddgaunt-gs requested review from mara-soldan and removed request for tembio October 16, 2025 13:50

toddgaunt-gs merged commit 293af49 into globalsign:master Nov 10, 2025
9 checks passed

Include challenge password attribute if required by EST server #38

Include challenge password attribute if required by EST server #38

Uh oh!

Conversation

mobe1 commented Mar 15, 2024

Uh oh!

toddgaunt-gs commented Mar 15, 2024

Uh oh!

toddgaunt-gs commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mobe1 commented Nov 12, 2024

Uh oh!

toddgaunt-gs commented Nov 12, 2024

Uh oh!

DDvO commented Apr 4, 2025

Uh oh!

DDvO commented Apr 4, 2025

Uh oh!

toddgaunt-gs commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddgaunt-gs commented Apr 9, 2025

Uh oh!

toddgaunt-gs commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mobe1 commented Apr 10, 2025

Uh oh!

mobe1 commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mobe1 commented Jul 15, 2025

Uh oh!

mobe1 commented Jul 15, 2025

Uh oh!

toddgaunt-gs commented Jul 15, 2025

Uh oh!

toddgaunt-gs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mobe1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mobe1 commented Sep 22, 2025

Uh oh!

mobe1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mobe1 commented Oct 7, 2025

Uh oh!

toddgaunt-gs commented Oct 16, 2025

Uh oh!

Uh oh!

Uh oh!

toddgaunt-gs commented Oct 16, 2025

Uh oh!

mobe1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mobe1 commented Oct 22, 2025

Uh oh!

Uh oh!

toddgaunt-gs commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddgaunt-gs commented Apr 9, 2024 •

edited

Loading

toddgaunt-gs commented Apr 9, 2025 •

edited

Loading

toddgaunt-gs commented Apr 9, 2025 •

edited

Loading

mobe1 commented Apr 10, 2025 •

edited

Loading

DDvO commented Apr 10, 2025 •

edited

Loading

toddgaunt-gs left a comment •

edited

Loading

toddgaunt-gs commented Nov 10, 2025 •

edited

Loading