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

Revoke-ACMECertificiate - Given Certificate as Input (rather than an order) #108

Open
GeorgeSchiro opened this issue Jan 5, 2021 · 43 comments
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed

Comments

@GeorgeSchiro
Copy link

Happy New Year!

In #13 we briefly discussed certificate revocation. You suggested that you could add an implementation that accepts the certificate itself as input (rather than an ACME order).

I responded "Clearly you must be given the private key. In what form? PFX file?"

Any feel for when you might work on this Thomas?

@glatzert
Copy link
Collaborator

glatzert commented Jan 6, 2021

Hi George,

ACME allows two ways to revoke the certificate:
Either via certificate pem and account key or
via certificate pem and certificate key.

The first one is implemented (via passing the order, which will (if not called with the paramters preventing it) store the certificate pem along the order itself) but untested.

The second one is currently not implemented, and I am not sure, HOW I would implement it.
I'll see, if I can convert the pfx to .der and use that.

I'll (try to) provide the following parameter sets:

  • State + Order
  • State + PEM
  • State + PFX
  • PFX

@GeorgeSchiro
Copy link
Author

Since we don't keep orders beyond their initial use (ie. acquiring the new certificate), we are in favor of the "PFX" approach.

We are already using OpenSSL to convert PFX to PEM (and vice-versa) to resolve another issue (don't recall the issue number), so we know a PEM file can contain the certificate's private key.

If the PEM already contains the certificate key, what did you mean by "or via certificate pem and certificate key" ?

@glatzert
Copy link
Collaborator

glatzert commented Jan 6, 2021

If the PEM already contains the certificate key, what did you mean by "or via certificate pem and certificate key" ?

if you want to revoke a certificate the request needs to be signed. Either with the account key (simple, since it's implemented) or via the certificate key (probably simple, but needs to be converted into Json Web Key).

I can implement State + PEM (public key only) very easily. Will do on the weekend.

@GeorgeSchiro
Copy link
Author

Can you get the certificate key (the private key, used for signing the revocation request) from the PEM as well?

@glatzert
Copy link
Collaborator

glatzert commented Jan 6, 2021

I will tell you as soon as I know the answer.

@glatzert
Copy link
Collaborator

glatzert commented Jan 7, 2021

It seems i can export the public key from the pfx, so it should be possible to implement it. (at least some tests around the matter have been successfull today)

@GeorgeSchiro
Copy link
Author

FYI, I found this: Revoking certificates (see "Using the certificate private key").

While a simple revocation call makes our lives easier, can you imagine the havoc a hacker could inflict if and when he gets a hold of your private key? It's not just about faking your site and swindling users (hard), but with access to a simple revocation mechanism a hacker can easily bring down your site by invalidating your certificate (again, assuming the private key has been compromised).

I really expected that revocation would have entailed a challenge from a site-owned server (similar to what's required for getting the certificate in the first place). But hey, what do I know!

@glatzert
Copy link
Collaborator

glatzert commented Jan 7, 2021

I think, if a hacker gets hold of your private key, the revocation of an ssl certificate might be one of the smaller problems your environment has (hey, thanks to LE you might just get a new cert).
Read about Ransom PKP, that's fun also ;)

@glatzert
Copy link
Collaborator

If you are feeling lucky, you might want to take a look into the dist folder of cert-revokation branch.
It currently needs the account key, since using the cert key is a bigger thing (I will implement that in the future, but it affects the Invoke-SignedRequest function, which everything depends upon...)

@GeorgeSchiro
Copy link
Author

Any sense for an ETA on the cert key implementation Thomas?

@glatzert
Copy link
Collaborator

Not yet, I have some work to do for my customers, which has priority over this (I think it's two weeks worth of work).
Nevertheless using the account key and the certificate should already work.

@glatzert
Copy link
Collaborator

glatzert commented Feb 9, 2021

So I was looking into the whole complex around using certificates to revoke a certificate and .. uff .. That's a lot of work - especially since I'd probably need more dependencies to NuGet packages, which will allow JW-Tokens, -Keys, etc. from X509Certificates.

So I won't make any promises about when that'll make it into the module.

@glatzert
Copy link
Collaborator

1.4.0-beta includes Revoke-AcmeCertificate with many different Parameter-Sets (all wildly untested). If you provide an PFX file or X509Certificate, that includes the private key, it will use that key (or should) to sign the request.

@GeorgeSchiro
Copy link
Author

Sorry for the delay getting to this Thomas. Would you prefer the typical bug report (ie. new issue)? Or would you prefer that I paste what I am seeing here?

@GeorgeSchiro
Copy link
Author

I'll just put it here for now (will move it later if you like):

Revoke-ACMECertificate

Cannot find the type for custom attribute 'Paremeter'. Make sure that the assembly that contains this type is loaded.
At C:\Users\admin\Desktop\GetCert2\ACME-PS\1.4.0\ACME-PS.psm1:2376 char:9
+         [Paremeter(ParameterSetName = "ByCert")]
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: ([Paremeter(ParameterSetName = "ByCert")]:AttributeAst) [], RuntimeException
    + FullyQualifiedErrorId : CustomAttributeTypeNotFound

@glatzert
Copy link
Collaborator

glatzert commented Mar 3, 2021 via email

@glatzert
Copy link
Collaborator

glatzert commented Mar 3, 2021

I assume you take the code from the dist folder? If yes, the fix is available.
Just go on with the problems here - we'll close it, if it works.
Thanks for your testing!

@GeorgeSchiro
Copy link
Author

Good news, we are no longer seeing an exception.

Bad news, we aren't see a PFX option either. Here's the currently supported parameters:

param(
    [Parameter(Mandatory = $true, Position = 0)]
    [ValidateNotNull()]
    [ValidateScript({$_.AccountExists()})]
    [AcmeState]
    $State,

    [Parameter(Mandatory = $true)]
    [ValidateNotNull()]
    [AcmeOrder]
    $Order
)

Here's what you wrote last month Thomas:

1.4.0-beta includes Revoke-AcmeCertificate with many 
different Parameter-Sets (all wildly untested). If you 
provide an PFX file or X509Certificate, that includes 
the private key, it will use that key (or should) to 
sign the request.

Looking at the latest in the dist folder, this is what we are seeing (in "ACME-PS.psd1"):

ModuleVersion = '1.3.3'

Does that make sense?

@glatzert
Copy link
Collaborator

@glatzert
Copy link
Collaborator

It's not yet in the master-branch

@GeorgeSchiro
Copy link
Author

Sorry Thomas. I've been running around today.

Wrong branch - well duh!

Ok, I can see it doesn't prompt for arguments this time with "Revoke-ACMECertificate" by itself.

Looking more closely at the code I can see something unexpected (before really thinking about it).

Here's what we were hoping for (perhaps naively):

Revoke-ACMECertificate -PFXCertificatePath "C:\File.pfx" -PFXCertificatePassword "FilePW"

That said, let me know if we need to create new state for the purpose of running just "Revoke-ACMECertificate", for example:

New-ACMEState      -Path "C:\AcmeState"
Get-ACMEServiceDirectory "C:\AcmeState" -ServiceName "LetsEncrypt" -PassThru
New-ACMENonce            "C:\AcmeState"
New-ACMEAccountKey       "C:\AcmeState"
New-ACMEAccount          "C:\AcmeState" -EmailAddresses "[email protected]" -AcceptTOS -PassThru

Revoke-ACMECertificate   "C:\AcmeState" -PFXCertificatePath "C:\File.pfx" -PFXCertificatePassword "FilePW"

Say the word and that's what we'll test. If there's a way to reduce some of the above, please let me know.

@glatzert
Copy link
Collaborator

The state is neccessary, since it's used to create urls and nonce - there is an in memory variant, which you can create with New-AcmeState (I think)
The AccountKey and Account should not be neccessary.

@GeorgeSchiro
Copy link
Author

NP, we will test without the account details. Thanks!

@GeorgeSchiro
Copy link
Author

Here are the preliminary results:

New-ACMEState      -Path "C:\AcmeState"
AcmeDiskPersistedState
Get-ACMEServiceDirectory "C:\AcmeState" -ServiceName "LetsEncrypt" -PassThru


ResourceUrl : https://acme-v02.api.letsencrypt.org/directory
NewAccount  : https://acme-v02.api.letsencrypt.org/acme/new-acct
NewAuthz    :
NewNonce    : https://acme-v02.api.letsencrypt.org/acme/new-nonce
NewOrder    : https://acme-v02.api.letsencrypt.org/acme/new-order
KeyChange   : https://acme-v02.api.letsencrypt.org/acme/key-change
RevokeCert  : https://acme-v02.api.letsencrypt.org/acme/revoke-cert
Meta        : AcmeDirectoryMeta



New-ACMENonce      "C:\AcmeState"

Revoke-ACMECertificate "C:\AcmeState" -PFXCertificatePath "C:\Cert.pfx" -PFXCertificatePassword "p"
You cannot call a method on a null-valued expression.
At C:\ACME-PS\1.4.0\ACME-PS.psm1:2445 char:69
+ ... rted X509 certificate key type ($($X509Cert.PrivateKey.GetType())).";
+                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : InvokeMethodOnNull

new : The term 'new' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that
the path is correct and try again.
At C:\ACME-PS\1.4.0\ACME-PS.psm1:2445 char:23
+                 throw new "Unsupported X509 certificate key type ($($ ...
+                       ~~~
    + CategoryInfo          : ObjectNotFound: (new:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

You cannot call a method on a null-valued expression.
At C:\ACME-PS\1.4.0\ACME-PS.psm1:481 char:9
+         $keyType = $keyParameters.GetType();
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : InvokeMethodOnNull


Confirm
Are you sure you want to perform this action?
Performing the operation "Revoking certificate." on target "Certificate".
[Y] Yes  [A] Yes to All  [N] No  [L] No to All  [S] Suspend  [?] Help (default is "Y"):

@GeorgeSchiro
Copy link
Author

Hey Thomas! I hope all is well. Can you please give me a sense for when you might have some time for this? Any feedback will be appreciated. Thanks!

@glatzert
Copy link
Collaborator

I have my test-platform up again, but did not yet have the try revoke certificates.
But I aim for within april.

@glatzert
Copy link
Collaborator

I just tested it, and it's .. difficult to say the least ..
The private key is read, but in Windows PS, it's null and is not available easily (need to be read from the cert:/ store AT LEAST).
In PS Core, it's available, but the private key parameters are not exportable (at least not easily).

This means to implement this, for PS-Core I'll need another code path to Invoke-ACMEWebRequest, which can work with [AssymetricAlgorithm]. And I'm not entirely sure, if I can do that, but that seems feasable.
Also, I don't see, how that would work on Windows Powershell AT ALL, since it does not load the private key.

I've found some errors, about the revocation with order and account key, which I'll solve, but I don't see how the other code-path would work.

@glatzert glatzert added bug Something isn't working enhancement New feature or request labels Apr 15, 2021
@glatzert glatzert added the help wanted Extra attention is needed label Apr 15, 2021
@GeorgeSchiro
Copy link
Author

Well that's disappointing Thomas.

Golly, since we can currently import any "Let's Encrypt" PFX file into any local certificate store, successfully bind it to any port and then use its private key for valid SSL requests, I would think you could do the same to get to the PFX's private key for revocation purposes. Why doesn't that make sense?

@glatzert
Copy link
Collaborator

It seems the certificate classes have not been build with files in mind, but certificates from the windows store - and this reflects here.

Which gives an additional opportunity to (windows users at least). It might be possible to read the cert directly from the store, when it's there ...

@glatzert
Copy link
Collaborator

Solved.

@GeorgeSchiro
Copy link
Author

Sorry Thomas. I meant to give it a look over the weekend. Got bogged down with other things. I will take a look soon. Thanks!

@glatzert
Copy link
Collaborator

I already tested it for Windows Powershell and it's running.
I revoked one cert using the order and one using the certificate private key.
I'll provide 1.5.0-beta via PSG during today or tomorrow

@GeorgeSchiro
Copy link
Author

Great! Were you able to make it work given just the PFX file (w/PW) and nothing else (other than state)? In other words, does the syntax still look like this:

Revoke-ACMECertificate "C:\AcmeState" -PFXCertificatePath "C:\Cert.pfx" -PFXCertificatePassword "p"

@glatzert
Copy link
Collaborator

Jep.
To be true, I did not test it with a password - only without. And I should probably have used a "fresh" state to be sure, it does not access the account-signing key.

You're guilty of having me rewrite the authentication-core of ACME-PS ;-)
I deleted a lot of low quality code.

@GeorgeSchiro
Copy link
Author

LOL

NP, I will test it with fresh state and password. Thanks Thomas!

@GeorgeSchiro
Copy link
Author

Well I finally got a chance to test your latest revocation fix. It works!

Note: earlier you felt that AccountKey and Account should not be necessary. If you are starting from scratch (ie. entirely new state) these values are indeed necessary. So here's what worked for me:

New-ACMEState      -Path "C:\AcmeState"
Get-ACMEServiceDirectory "C:\AcmeState" -ServiceName "LetsEncrypt" -PassThru
New-ACMENonce            "C:\AcmeState"
New-ACMEAccountKey       "C:\AcmeState"
New-ACMEAccount          "C:\AcmeState" -EmailAddresses "[email protected]" -AcceptTOS -PassThru

Revoke-ACMECertificate   "C:\AcmeState" -PFXCertificatePath "C:\Cert.pfx" -PFXCertificatePassword "p"

Thank you for doing this Thomas. Your efforts are greatly appreciated.

@glatzert
Copy link
Collaborator

glatzert commented May 9, 2021 via email

@glatzert
Copy link
Collaborator

Yep - definately a bug:

[Parameter(Mandatory = $true, Position = 0)]
[ValidateNotNull()]
[ValidateScript({$_.AccountExists()})]
[AcmeState]
$State,

But simple to fix.

@glatzert glatzert reopened this May 10, 2021
@GeorgeSchiro
Copy link
Author

GeorgeSchiro commented May 29, 2021

Hey Thomas! We are ready to start full integration and regression testing the latest version of ACME-PS (we are still using version 1.1.5 in our production systems). It would be nice to test the new revocation feature (sans the account stuff). Do you have a sense for when we might be able to do that?

@glatzert
Copy link
Collaborator

glatzert commented Jun 1, 2021

It's not in the PSG, yet, but you can get 1.5.1 from /dist/

@glatzert glatzert closed this as completed Jun 1, 2021
@GeorgeSchiro
Copy link
Author

Thank you Thomas.

I ran this test through the staging system:

Import-Module            "C:\ACME-PS"
New-ACMEState      -Path "C:\AcmeState"
Get-ACMEServiceDirectory "C:\AcmeState" -ServiceName "LetsEncrypt-Staging" -PassThru

Revoke-ACMECertificate   "C:\AcmeState" -PFXCertificatePath "C:\Cert.pfx" -PFXCertificatePassword "p"

Here's what I am seeing:

The variable cannot be validated because the value $null is not a valid value for the SigningKey variable.
At C:\ACME-PS\1.5.1\ACME-PS.psm1:3506 char:13
+             $signingKey = $State.GetAccountKey();
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : MetadataError: (:) [], ValidationMetadataException
    + FullyQualifiedErrorId : ValidateSetFailure

New-SignedMessage : Cannot bind argument to parameter 'SigningKey' because it is null.
At C:\ACME-PS\1.5.1\ACME-PS.psm1:3511 char:64
+ ... estBody = New-SignedMessage -Url $Url -SigningKey $signingKey -KeyId  ...
+                                                       ~~~~~~~~~~~
    + CategoryInfo          : InvalidData: (:) [New-SignedMessage], ParameterBindingValidationException
    + FullyQualifiedErrorId : ParameterArgumentValidationErrorNullNotAllowed,New-SignedMessage

Server returned Problem (Status: 400).
Type: urn:ietf:params:acme:error:malformed
Parse error reading JWS
At C:\ACME-PS\1.5.1\ACME-PS.psm1:3526 char:13
+             throw [AcmeHttpException]::new($response.ErrorMessage, $r ...
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (:) [], AcmeHttpException
    + FullyQualifiedErrorId : Server returned Problem (Status: 400).
Type: urn:ietf:params:acme:error:malformed
Parse error reading JWS

@GeorgeSchiro
Copy link
Author

Anyway, everything else checks out fine, including revocation using the account details (as described on May 8th, see above). So we will proceed with 1.5.1 as it is. Thanks again Thomas.

@glatzert
Copy link
Collaborator

glatzert commented Jun 7, 2021

The error message indicates, it did not call the code path for an existing private key, thus it tried to get the account key from the state.

@glatzert glatzert reopened this Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants