Skip to content

Conversation

@christianhoelzl
Copy link
Contributor

@christianhoelzl christianhoelzl commented Apr 8, 2025

This pull request adds account features and one fix to the instant-acme client:

  • A new feature is added to update the contact information of an existing acme account
  • A new feature is added to perform an account key rollover of an existing acme account

The contact information that is part of the acme account is according to RFC 8555, section 7.3.2. The contact update payload includes a the contact information as for example an email address.

The account key rollover is according to RFC 8555, section 7.3.5. New credentials are created by the instant-acme client and updated on the acme server. The new credentials are returned to the caller for further usage.

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I think we'll want some tests for this in the pebble integration test. Please squash any fixup commits.

Would be helpful to add links to the specific RFC sections in the docstring/comments.

@christianhoelzl
Copy link
Contributor Author

Documentation of the new API methods is added to the last commit with links into RFC 8555.

You mentioned a pebble integration test. I need some guidance regarding pebble. Where should a testcase be placed in pebble.rs? Not shure if pebble supports contacts update or account key rollover.

@djc
Copy link
Owner

djc commented Apr 8, 2025

Just add new tests below the list existing test functions. Suggest just trying out whether Pebble supports these endpoints.

@christianhoelzl
Copy link
Contributor Author

Two pebble tests are added to this pull request. I tested locally on windows OS with the latest pebble release.

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks! A few more small issues.

Copy link
Collaborator

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Cool :-)

@christianhoelzl
Copy link
Contributor Author

Most review changes are fixed in this commit. I extended the readme to show the new features.
Still open is the discussion on Problem::from_response and the discussion on the pebble test for update contacts.

@cpu
Copy link
Collaborator

cpu commented Apr 13, 2025

Still open is the discussion on Problem::from_response and the discussion on the pebble test for update contacts.

I left a comment about the issue for the pebble test for contacts update. I think you just need to fix the contact you're using in the update.

Copy link
Collaborator

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks for working through all the review feedback. Looks great.

@cpu
Copy link
Collaborator

cpu commented Apr 16, 2025

@djc Thinking we should squash merge this one, oui? Do you want to do the honours?

@djc djc merged commit 55449a1 into djc:main Apr 16, 2025
9 checks passed
@djc
Copy link
Owner

djc commented Apr 16, 2025

Fait accompli!

@christianhoelzl
Copy link
Contributor Author

@djc and @cpu, thank you for merging this pull request. This was my very first public pull request. For me it was fun working through your comments and contributing to instant-acme.

@cpu
Copy link
Collaborator

cpu commented Apr 16, 2025

This was my very first public pull request.

That's awesome. Thanks for sharing :-) You did a great job.

@djc
Copy link
Owner

djc commented Apr 16, 2025

Thanks for your contribution! Hopefully this left you feeling motivated to contribute more in the future!

@djc djc mentioned this pull request Apr 23, 2025
@christianhoelzl christianhoelzl deleted the acme_account_features branch July 9, 2025 10:20
@cpu cpu mentioned this pull request Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants