public inbox for pmg-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>
Cc: pmg-devel@lists.proxmox.com
Subject: Re: [pmg-devel] [PATCH api 5/8] api: add ACME and ACMEPlugin module
Date: Fri, 12 Mar 2021 15:10:15 +0100	[thread overview]
Message-ID: <20210312141015.sisnae3m6k3zi556@wobu-vie.proxmox.com> (raw)
In-Reply-To: <1a9960dd-ef4f-e348-ea73-30272b9df8cd@proxmox.com>

On Thu, Mar 11, 2021 at 11:41:22AM +0100, Dominik Csapak wrote:
> comments inline
> 
(...)
> > @@ -0,0 +1,436 @@
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'account_index',
> > +    path => 'account',
> > +    method => 'GET',
> > +    permissions => { user => 'all' },
> 
> i'd argue that the qmanager should not list the
> available acme accounts

right

> 
> > +    description => "ACME account index.",
> > +    protected => 1,
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'array',
> > +	items => {
> > +	    type => "object",
> > +	    properties => {},
> > +	},
> > +	links => [ { rel => 'child', href => "{name}" } ],
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $accounts = PMG::CertHelpers::list_acme_accounts();
> > +	return [ map { { name => $_ }  } @$accounts ];
> > +    }});
> 
> for the following create/update
> the permissions are missing but should be 'admin'
> (they are ok for the plugins)

yeah, fixing

> 
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'register_account',
> > +    path => 'account',
> > +    method => 'POST',
> > +    description => "Register a new ACME account with CA.",
> > +    proxyto => 'master',
> > +    protected => 1,
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    name => get_standard_option('pmg-acme-account-name'),
> > +	    contact => get_standard_option('pmg-acme-account-contact'),
> > +	    tos_url => {
> > +		type => 'string',
> > +		description => 'URL of CA TermsOfService - setting this indicates agreement.',
> > +		optional => 1,
> > +	    },
> > +	    directory => get_standard_option('pmg-acme-directory-url', {
> > +		default => $acme_default_directory_url,
> > +		optional => 1,
> > +	    }),
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $rpcenv = PMG::RESTEnvironment->get();
> > +	my $authuser = $rpcenv->get_user();
> > +
> > +	my $account_name = extract_param($param, 'name') // 'default';
> > +	my $account_file = "${acme_account_dir}/${account_name}";
> > +	mkdir $acme_account_dir if ! -e $acme_account_dir;
> > +
> > +	raise_param_exc({'name' => "ACME account config file '${account_name}' already exists."})
> > +	    if -e $account_file;
> > +
> > +	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
> > +	my $contact = $account_contact_from_param->($param);
> > +
> > +	my $realcmd = sub {
> > +	    PMG::CertHelpers::lock_acme($account_name, 10, sub {
> > +		die "ACME account config file '${account_name}' already exists.\n"
> > +		    if -e $account_file;
> > +
> > +		print "Registering new ACME account..\n";
> > +		my $acme = PMG::RS::Acme->new($directory);
> > +		eval {
> > +		    $acme->new_account($account_file, defined($param->{tos_url}), $contact, undef);
> > +		};
> > +		if (my $err = $@) {
> > +		    unlink $account_file;
> > +		    die "Registration failed: $err\n";
> > +		}
> > +		my $location = $acme->location();
> > +		print "Registration successful, account URL: '$location'\n";
> > +	    });
> > +	    die $@ if $@;
> > +	};
> > +
> > +	return $rpcenv->fork_worker('acmeregister', undef, $authuser, $realcmd);
> > +    }});
> > +
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'deactivate_account',
> > +    path => 'account/{name}',
> > +    method => 'DELETE',
> > +    description => "Deactivate existing ACME account at CA.",
> > +    protected => 1,
> > +    proxyto => 'master',
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    name => get_standard_option('pmg-acme-account-name'),
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	return $update_account->($param, 'deactivate', status => 'deactivated');
> > +    }});
> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'get_tos',
> > +    path => 'tos',
> > +    method => 'GET',
> > +    description => "Retrieve ACME TermsOfService URL from CA.",
> > +    permissions => { user => 'all' },
> > +    parameters => {
> > +	additionalProperties => 0,
> > +	properties => {
> > +	    directory => get_standard_option('pmg-acme-directory-url', {
> > +		default => $acme_default_directory_url,
> > +		optional => 1,
> > +	    }),
> > +	},
> > +    },
> > +    returns => {
> > +	type => 'string',
> > +	optional => 1,
> > +	description => 'ACME TermsOfService URL.',
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $directory = extract_param($param, 'directory') // $acme_default_directory_url;
> > +
> > +	my $acme = PMG::RS::Acme->new($directory);
> > +	my $meta = $acme->get_meta();
> > +
> > +	return $meta ? $meta->{termsOfService} : undef;
> > +    }});
> 
> just for my understanding: what happens here if there is no TOS?
> is that valid ACME behaviour? or should we somehow error out?

According to the RFC the value is optional and so we should not error
out.

> > +__PACKAGE__->register_method({
> > +    name => 'add_plugin',
> > +    path => '',
> > +    method => 'POST',
> > +    description => "Add ACME plugin configuration.",
> > +    permissions => { check => [ 'admin' ] },
> > +    protected => 1,
> > +    parameters => PVE::ACME::Challenge->createSchema(),
> > +    returns => {
> > +	type => "null"
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $id = extract_param($param, 'id');
> > +	my $type = extract_param($param, 'type');
> > +
> > +	lock_config(sub {
> > +	    my $cfg = load_config();
> > +	    die "ACME plugin ID '$id' already exists\n" if defined($cfg->{ids}->{$id});
> > +
> > +	    my $plugin = PVE::ACME::Challenge->lookup($type);
> > +	    my $opts = $plugin->check_config($id, $param, 1, 1);
> > +
> > +	    $cfg->{ids}->{$id} = $opts;
> > +	    $cfg->{ids}->{$id}->{type} = $type;
> > +
> > +	    write_config($cfg);
> > +	});
> > +	die "$@" if $@;
> 
> you already die in lock_config if $@ is set.

fixing all those up

> > +
> > +	return undef;
> > +    }
> > +});
> > +
> > +__PACKAGE__->register_method({
> > +    name => 'update_plugin',
> > +    path => '{id}',
> > +    method => 'PUT',
> > +    description => "Update ACME plugin configuration.",
> > +    permissions => { check => [ 'admin' ] },
> > +    protected => 1,
> > +    parameters => PVE::ACME::Challenge->updateSchema(),
> > +    returns => {
> > +	type => "null"
> > +    },
> > +    code => sub {
> > +	my ($param) = @_;
> > +
> > +	my $id = extract_param($param, 'id');
> > +	my $delete = extract_param($param, 'delete');
> > +	my $digest = extract_param($param, 'digest');
> > +
> > +	lock_config(sub {
> > +	    my $cfg = load_config();
> > +	    PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
> > +	    my $plugin_cfg = $cfg->{ids}->{$id};
> > +	    die "ACME plugin ID '$id' does not exist\n" if !$plugin_cfg;
> > +
> > +	    my $type = $plugin_cfg->{type};
> > +	    my $plugin = PVE::ACME::Challenge->lookup($type);
> > +
> > +	    if (defined($delete)) {
> > +		my $schema = $plugin->private();
> > +		my $options = $schema->{options}->{$type};
> > +		for my $k (PVE::Tools::split_list($delete)) {
> > +		    my $d = $options->{$k} || die "no such option '$k'\n";
> > +		    die "unable to delete required option '$k'\n" if !$d->{optional};
> > +
> > +		    delete $cfg->{ids}->{$id}->{$k};
> > +		}
> > +	    }
> > +
> > +	    my $opts = $plugin->check_config($id, $param, 0, 1);
> > +	    for my $k (sort keys %$opts) {
> 
> not that it should make a difference, but why sort?

PVE copy-pasta ;-) will fix




  reply	other threads:[~2021-03-12 14:10 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-09 14:13 [pmg-devel] [RFC api/gui/wtk/acme 0/many] Certificates & ACME Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 1/8] depend on libpmg-rs-perl and proxmox-acme Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 2/8] add PMG::CertHelpers module Wolfgang Bumiller
2021-03-11 10:05   ` Dominik Csapak
2021-03-12 13:55     ` Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 3/8] add PMG::NodeConfig module Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 4/8] cluster: sync acme/ and acme-plugins.conf Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 5/8] api: add ACME and ACMEPlugin module Wolfgang Bumiller
2021-03-11 10:41   ` Dominik Csapak
2021-03-12 14:10     ` Wolfgang Bumiller [this message]
2021-03-09 14:13 ` [pmg-devel] [PATCH api 6/8] add certificates api endpoint Wolfgang Bumiller
2021-03-11 11:06   ` Dominik Csapak
2021-03-12 14:51     ` Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 7/8] add node-config api entry points Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH api 8/8] add acme and cert subcommands to pmgconfig Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH gui] add certificates and acme view Wolfgang Bumiller
2021-03-11 12:35   ` Dominik Csapak
2021-03-09 14:13 ` [pmg-devel] [PATCH acme] add missing 'use PVE::Acme' statement Wolfgang Bumiller
2021-03-12 15:00   ` [pmg-devel] applied: " Thomas Lamprecht
2021-03-09 14:13 ` [pmg-devel] [PATCH widget-toolkit 1/7] Utils: add ACME related utilities Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH widget-toolkit 2/7] add ACME related data models Wolfgang Bumiller
2021-03-11 12:41   ` Dominik Csapak
2021-03-09 14:13 ` [pmg-devel] [PATCH widget-toolkit 3/7] add ACME forms: Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH widget-toolkit 4/7] add certificate panel Wolfgang Bumiller
2021-03-09 14:13 ` [pmg-devel] [PATCH widget-toolkit 5/7] add ACME account panel Wolfgang Bumiller
2021-03-11 13:51   ` Dominik Csapak
2021-03-11 15:14     ` Thomas Lamprecht
2021-03-11 15:16       ` Dominik Csapak
2021-03-11 15:27         ` Thomas Lamprecht
2021-03-09 14:14 ` [pmg-devel] [PATCH widget-toolkit 6/7] add ACME plugin editing Wolfgang Bumiller
2021-03-09 14:14 ` [pmg-devel] [PATCH widget-toolkit 7/7] add ACME domain editing Wolfgang Bumiller
2021-03-10 12:27 ` [pmg-devel] [RFC api/gui/wtk/acme 0/many] Certificates & ACME Dominik Csapak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210312141015.sisnae3m6k3zi556@wobu-vie.proxmox.com \
    --to=w.bumiller@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pmg-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal