Re: [pmg-devel] [PATCH api 5/8] api: add ACME and ACMEPlugin module

[Wolfgang Bumiller <w.bumiller@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