From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9511169ECB for ; Fri, 12 Mar 2021 15:10:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 89FF833710 for ; Fri, 12 Mar 2021 15:10:17 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id AFAA433705 for ; Fri, 12 Mar 2021 15:10:16 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 735B2463CF for ; Fri, 12 Mar 2021 15:10:16 +0100 (CET) Date: Fri, 12 Mar 2021 15:10:15 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pmg-devel@lists.proxmox.com Message-ID: <20210312141015.sisnae3m6k3zi556@wobu-vie.proxmox.com> References: <20210309141401.19237-1-w.bumiller@proxmox.com> <20210309141401.19237-6-w.bumiller@proxmox.com> <1a9960dd-ef4f-e348-ea73-30272b9df8cd@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1a9960dd-ef4f-e348-ea73-30272b9df8cd@proxmox.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pmg-devel] [PATCH api 5/8] api: add ACME and ACMEPlugin module X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Mar 2021 14:10:17 -0000 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