intrigeri writes ("Bug#932570: dgit should pin to the LE CA for ftpmasterapi"):
> Here's a first proof-of-concept on the 932570-pin-LetsEncrypt-CA
> branch on https://salsa.debian.org/intrigeri/dgit.Hi. Thanks a lot. I have looked at your mail and code in detail now. First there is some discussion and then later code review. I think this is all very promising and also important. I am happy to do all this by email but maybe IRL conversation would work too, or better. So do try to catch me around the conference... > This assumes we ship the Let's Encrypt CA in the dgit package until > #932590 is fixed; once that CA is shipped by ca-certificates, we can > point dgit-distro.debian.archive-query-tls-cacert to it and stop > shipping the CA ourselves. SGTM. (Sounds Good To Me) > What I did not do: > > - I did not find how to run the full test suite so this might > break stuff. Sorry about the lack of a HACKING document. There's one in salsa master now. That is, here: https://salsa.debian.org/dgit-team/dgit (and I'm fixing Vcs-Git too). TBH I am very happy to fix any bugs in this important contribution. (Bugfixing someone else's code is a really good way to do code review too...) > - I did not write tests :/ > > Is the test suite operating fully offline or is it already mocking > remote services? It operates fully offline. The mock of the ftpmaster api involves a maintaining a filesystem directory tree containing api responses, and specifying the api url as a file:// url. I doubt we want to change this, because it is pleasingly simple and fast. I think we should therefore add a new test case for the cert stuff. I guess it would need to (1) set up a thing that looks enough like the cert structure from LE that it is a valid test (ie do we want the key we verify to be an intermediate? That will detect if libcurl stops behaving the way with want.) (2) run up some kind of minimal https server (it would have to allow the kernel to select the port so that multiple ad-hoc instances of the test suite can work simulataneously, and that will complicate the rest of the test because the port will have to go into the next part...) (3) run a couple of dgit archive-api-query runes with different configs to check the various success and failure cases. I'd like to talk to you IRL about how to achieve (1) and (2), which I don't think I know how to do (at least, without doing some reading...) > - I'm no good with Makefile's so my (untested) attempt at installing > the CA in the expected directory is probably buggy. I can fix all that stuff, no problem. Finally, re your patch: Remove unused cmd_archive_api_query function. Unfortunately this is not unused. cmd_XXX functions correspond to dgit subcommands. This one is not documented but it is used internally by the dgit-infrastructure server side stuff. I think this means that cmd_archive_api_query needs to be refactored so that it calls a variant of api_query (specifically, a variant that does not do the json parsing). That probably wants to come earlier in the series, to retain bisectability. Right, on to the code: > +package Debian::Dgit::ArchiveAPIQuery; I don't know why you wanted to introduce this as a separate perl module. Not that I really mind, but would you care to explain ? Also, and I won't be picky about this here because I think this is all really important, but I generally prefer reorganisation/refactoring to be in separate commits. I like my commits to be much smaller than most people do. So I would be inclined to split off the module first, if that would work. If I had done this I might well do: 1. split up api_query into api_query_raw that doesn't do decode json, which is called by a small new api_query 2. make cmd_archive_api_query use api_query, so now archive_api_query_cmd has one caller 3. abolish archive_api_query_cmd and make api_query_raw use LWP rather than execing curl etc. And presumbly if a new module is wanted, introduce it after 2 or after 3. If you like I could prepare a branch with 1 and 2 already done. Would that be helpful ? Also you will find that introducing a new perl module may cause build system lossage because of some strange practices in the build system. I mention this not to get you to fix it - but rather, so that you will know it's not your fault and ignore it. I'll fix it :-). > + my $curl = WWW::Curl::Easy->new; Does this need an "or die" ? > + my $response_body; > + $curl->setopt(CURLOPT_URL, $url); ... Likewise. > + my $retcode = $curl->perform; > + $retcode == 0 or I think maybe you mean "eq 0" or to add an "// die" or something. Since maybe $curl->perform might return undef ? > diff --git a/Lets-Encrypt-Authority-X3.pem b/Lets-Encrypt-Authority-X3.pem I think I might put this in a certs/ subdirectory. I am happy to fix that up along with all the other build system stuff. > +certsdir=$(prefix)/share/dgit/certs Shouldn't this be in /etc somewhere ? IDK how this is normally done. > - libtext-iconv-perl, libtext-glob-perl > + libtext-iconv-perl, libtext-glob-perl, > + libwww-curl-perl I guess this is available in stretch without difficulty. > # ^ this is a workaround but works (only) on DSA-administered machines > +# Meanwhile, until #790093 is fixed, let's at least fix #932570: > + 'dgit-distro.debian.archive-query-tls-cacert' => > + '/usr/share/dgit/certs/Lets-Encrypt-Authority-X3.pem', Presumably we need to be able to support multiple certs ? Ie if LE say they are going to roll their key over, we will want to immediately put the new key as well as the old one in our .deb. Can we just concatenate the multiple certs in one .pem or do we have to have a directory or iterate ourself or something ? > + if ($url =~ m#^https://([-.0-9a-z]+)/#) { > + # Until #790093 is fixed, let's at least fix #932570: > + $cacert = access_cfg('archive-query-tls-cacert','RETURN-UNDEF') //''; > + if (!stat $cacert) { > + fail "for $url: stat $cacert: $!" unless $!==ENOENT; > + } I don't think I understand what this stat is for. If we pass an unreadable or nonexistent thing to libcurl, presumably it will fail as desired ? If not then maybe we want Dgit::stat_exists, depending on the actually desired semantics. Right, should send this email and go and have some lunch... Thanks, Ian.
