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

Make email optional #15

Closed
wants to merge 2 commits into from
Closed

Make email optional #15

wants to merge 2 commits into from

Conversation

kpcyrd
Copy link

@kpcyrd kpcyrd commented Apr 28, 2020

This also contains code to allow accessing the generated certificate without sending it to persistence. I've tested this with the staging acme servers.

Copy link
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

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

Hi Thanks for making this.

I'm a bit perplexed as to what's driving the change though? I see that contact details are indeed optional in the acme spec. https://www.rfc-editor.org/rfc/rfc8555#section-7.3 But as you noticed, this lib is somewhat designed to have the contact email as the entry point around which a lot else hinges.

It's a breaking API change, so it needs to be clearly motived and also put some other (required) mechanism in place for the "realm" towards the persistence.

Furthermore no doc changes were made to reflect the API change. However before doing that, we need to establish how/and why. Again it's not a definite "no", but let's discuss.

@kpcyrd
Copy link
Author

kpcyrd commented May 4, 2020

I'm trying to use acme-lib in a more low-level way, so I'm trying to bypass everything that's related to persistence and only use the "raw", direct functions for acme.

I noticed that acme-lib is aiming to be more high-level, but also the only acme v2 crate for rust that I could find. Having all the low-level functions exposed while also keeping a high-level abstraction (that I don't have to opt-into) would be ideal for me.

If that's not a direction you're interested in taking I'm also open to maintain a fork if you don't mind. :)

@algesten
Copy link
Owner

algesten commented May 6, 2020

Thanks I understand now. I think we can make this work, but maybe by not modifying the current surface API.

Downloading a cert without persistence maybe is solvable using the MemoryPersist and existing API?

I propose changing it like this: 1e1a31a

Just checking if that does what you need and then I release a new crate?

@kpcyrd
Copy link
Author

kpcyrd commented May 6, 2020

That works for me, any chance we can also allow direct access to:

Right now I have to work around the existing storage like this:

fn try_load_acc(persist: &MyPersist, mem: &MemoryPersist) -> Result<bool> {
    if let Some(acc) = persist.load_acc_privkey()? {
        let p = PersistKey::new(REALM, PersistKind::AccountPrivateKey , REALM);
        mem.put(&p, acc.as_bytes()).unwrap();
        Ok(true)
    } else {
        Ok(false)
    }
}

fn get_acc_key(mem: &MemoryPersist) -> String {
    let p = PersistKey::new(REALM, PersistKind::AccountPrivateKey , REALM);
    let privkey = mem.get(&p).unwrap().unwrap();
    String::from_utf8(privkey).unwrap()
}

// Create a directory entrypoint.
let mem = MemoryPersist::new();

let already_existed = try_load_acc(&persist, &mem)?;
let dir = Directory::from_url(mem.clone(), url)?;

info!("authenticating with account");
let acc = dir.account_with_realm(REALM, vec![])?;
if !already_existed {
    info!("saving private key for newly registered account");
    let privkey = get_acc_key(&mem);
    persist.store_acc_privkey(&privkey)?;
}
// do something with `acc`

instead I'd rather:

let dir = Directory::from_url(MemoryPersist::new(), url)?;
let acc = if let Some(acc) = persist.load_acc_privkey()? {
    info!("authenticating with existing account");
    dir.account_from_str(&acc)?
} else {
    info!("registering account");
    let acc = dir.register_account();
    info!("successfully created account, saving private key");
    persist.store_acc_privkey(&acc.private_key())?;    
};
// do something with `acc`

I'm still surprised that I have to deal with realm even though that concept doesn't exist in my application, other acme clients or acme itself. I'm wondering if it would make more sense to just go with per-account persistence and then move the concept of a realm into persistence:

dir.account_with_realm(FilePersist::new("/some/path", "some_realm"), vec![])?;

@algesten
Copy link
Owner

algesten commented May 7, 2020

Since you ultimately want to save keys and certificates, wouldn't it just be easier to implement the Persist trait?

@algesten
Copy link
Owner

algesten commented May 7, 2020

@kpcyrd Since it seems you do want to persist the keys/certificates, can we maybe explore why the Persist trait isn't working for you in #18?

kpcyrd referenced this pull request in kpcyrd/acme-micro Apr 5, 2022
@algesten algesten closed this Jan 4, 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.

2 participants