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.

Sign up for GitHub

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

Jump to bottom

ACMEAccountService fixes/enhancements #4625

Merged
merged 2 commits into from
Nov 30, 2023
Merged

ACMEAccountService fixes/enhancements #4625

merged 2 commits into from
Nov 30, 2023

Conversation

frasertweedale
Copy link
Contributor

@frasertweedale frasertweedale commented Nov 28, 2023 •
edited
Loading 

e2046ef87c (Fraser Tweedale, 7 minutes ago)
   acme: implement POST-as-GET for accounts

   Some ACME clients including mod_md POST-as-GET the account resource
   (for an existing account), expected to read the account info.  In 
   particular, mod_md does this and certificate renewal fails because it
   cannot read or verify the account information.

   The ACME protocol does not explicitly require this behaviour.  But on the
   other hand, it is not surprising that clients assume they can do it, and it
   arguably is surprising if an ACME server does not provide it.

   So let's implement it.  The change itself is trivial: when payload is
   empty, POST-as-GET is implied (RFC 8555 section 6.3).  In this case, return
   the ACMEAccount object (which we already have at hand) unchanged.

48a385fcc4 (Fraser Tweedale, 28 minutes ago)
   acme: return proper error on malformed account payload

   ACMEAccountService currently throws an uncaught exception if decode the
   account object payload fails.  This results in the server responding 500
   Internal Server Error.  Respond with status 400 and a proper problem
   document instead.

@frasertweedale
acme: return proper error on malformed account payload
48a385f 
ACMEAccountService currently throws an uncaught exception if decode
the account object payload fails.  This results in the server
responding 500 Internal Server Error.  Respond with status 400 and
a proper problem document instead.
@frasertweedale
acme: implement POST-as-GET for accounts
ed31277 
Some ACME clients POST-as-GET the account resource, expecting to
receive the account object (for an existing account).  In
particular, mod_md does this and certificate renewal fails when it
cannot read or verify the account information.

The ACME protocol does not explicitly require this behaviour.  But
on the other hand, it is not surprising that clients assume they can
do it, and it arguably is surprising if an ACME server does not
provide it.

So let's implement it.  The change itself is trivial: when payload
is empty, POST-as-GET is implied (RFC 8555 section 6.3).  In this
case, return the ACMEAccount object (which we already have at hand)
unchanged.
@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
13.1% 13.1% Duplication

fmarco76
fmarco76 approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Reading the rfc this behaviour seems expected.
@edewata: should we backport this to other branches.

@fmarco76
Copy link
Member

There is a KRA failure but I do not see links with the changes in this PR so I would accept and investigate that problem in separate PR

@edewata
Copy link
Contributor

edewata commented Nov 29, 2023

The KRA failure disappeared after I restarted the test.

edewata
edewata approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch looks good. Once it's merged to master branch this patch will be included in the next build in Fedora 40, RHEL 9.4/10, and RHCS 10.6. About backporting, I don't think this is needed in older RHCS versions and IIUC ACME will remain a tech preview in RHEL 8. But if IPA needs it in older RHEL versions, we can certainly backport the patch and create a new build, just file a RHEL Jira ticket for that.

@fmarco76 fmarco76 merged commit eb5db84 into dogtagpki:master Nov 30, 2023
228 of 230 checks passed
@fmarco76
Copy link
Member

@frasertweedale @edewata Thanks!

@frasertweedale frasertweedale deleted the fix/acme-get-account branch November 30, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edewata edewata edewata approved these changes

@fmarco76 fmarco76 fmarco76 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frasertweedale @fmarco76 @edewata