Page MenuHomeFreeBSD

Extend security/ca_root_nss MAca-bundle.pl
Needs ReviewPublic

Authored by allanjude on Aug 11 2018, 6:14 PM.

Details

Summary

This patch extends the MAca-bundle.pl script to optionally split the certdata.txt into individual files
named for the certificate authority.

This is planned to be checked into the src tree for use as the caroot-in-base feature, but this will
keep them in sync.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19047
Build 18678: arc lint + arc unit

Event Timeline

allanjude created this revision.Aug 11 2018, 6:14 PM
netchild added inline comments.Aug 11 2018, 8:59 PM
security/ca_root_nss/files/MAca-bundle.pl.in
202

I suggest to add a comment why this is necessary. Same for line 225.

mandree requested changes to this revision.Aug 12 2018, 9:27 AM

The patch isn't clean, not against the 2013 version of my script in SVN - if I download the patch here, I cannot apply it, the second hunk fails, and wiggling it leads to a merge failure. Apparently the patch was not created against what is currently in SVN but a locally modified version. When editing the code as suggested by the comments above, please recreate the patch against what is in SVN.

security/ca_root_nss/files/MAca-bundle.pl.in
39

I wonder if we should have long options as well. Especially the -o appears to be non-obvious as output directory rather than output file. I understand that -d is taken for debug.

54

We need to print a help text if unknown options or arguments are used.

68

The needs a prototype added, apparently ($$) is needed.

73

A function should not second-guess options. All options parsing should be in the scope that calls Getopt::Whatever(), i. e. in this case in the global scope.

100

Umm... we break the first argument out as apparent file handle ($fh), and then use OUT? Looks inconsistent to me in ll. 116, 118, 119 below.

107–108

It appears that in the previous and this iteration of the code, we do not need this function any more - we could just delete it, and rename printcert_info($$$) above to printcert($$$), or am I missing something?

108

$infile should become an argument to this function -- used in l. 131 below.

124

$infile should become an argument to this function -- used in l. 148 below.

150–151

$infile should become an argument to this function, used on line 173 below.

251

This lacks error handling (out of disk space, lack of permission, ...). Please add. (I'll concede that the previous version of the code did not check stdout write success either...)

257

we need to add a close($fh) and error checks here.

270

Hmmm... we use this marker inconsistently depending on whether -o DIR is used or not - is that intentional?

This revision now requires changes to proceed.Aug 12 2018, 9:27 AM
mat added inline comments.Aug 13 2018, 11:50 AM
security/ca_root_nss/files/MAca-bundle.pl.in
68

From Perl::Critic::Policy::Freenode::Prototypes

Function prototypes are primarily a hint to the Perl parser for parsing the function's argument list. They are not a way to validate or count the arguments passed to the function, and will cause confusion if used this way.

Just do not use prototypes.

mandree added inline comments.Aug 13 2018, 11:25 PM
security/ca_root_nss/files/MAca-bundle.pl.in
68

Whatever the camel brain aka. Perl engine does with it - they help the reader of the code during maintenance, and should be used for consistency with other parts of the code.

allanjude marked 14 inline comments as done.Aug 14 2018, 3:22 PM
allanjude added inline comments.
security/ca_root_nss/files/MAca-bundle.pl.in
73

In this case we are changing the header in the output file to make sense when each output file contains only one cert, not all 150 of them.

100

This OUT is a different file descriptor, it is a pipe to openssl where we write the DER encoded data, and get back the text and PEM encoded certificate.

108

It was a global variable, I am not sure there is much value in passing it in instead, but done.

202

I think it is close enough in the code to only need the comment once.

270

Yes, having the count in each file doesn't make sense. Each file in the output will contain exactly 1 certificate.

allanjude updated this revision to Diff 46665.Aug 14 2018, 3:23 PM
allanjude marked 4 inline comments as done.

Update with feedback from mandree

mandree requested changes to this revision.Aug 14 2018, 8:47 PM

Thanks. This is looking almost good, just the error handling for the close() and we should be done. I haven't been able to do run-time testing yet.

Again, the downloaded diff does not apply - what's wrong here?

--------------------------
|Index: security/ca_root_nss/files/MAca-bundle.pl.in
|===================================================================
|--- security/ca_root_nss/files/MAca-bundle.pl.in
|+++ security/ca_root_nss/files/MAca-bundle.pl.in
--------------------------
Patching file files/MAca-bundle.pl.in using Plan A...
Hunk #1 succeeded at 1.
Hunk #2 failed at 36.
Hunk #3 succeeded at 92.
Hunk #4 succeeded at 121.
Hunk #5 succeeded at 188.
1 out of 5 hunks failed while patching files/MAca-bundle.pl.in
done
security/ca_root_nss/files/MAca-bundle.pl.in
100

oh, right, missed that it is local. My apologies.

108

Thanks. As we make the code more complex, making it more maintainable seemed to make sense to me.

256

this is a place where error checking will also be beneficial - it will report write errors that only occur once we have the buffers flushed.

... or die "SOME FANCY ERROR MESSAGE WITH REASON";
This revision now requires changes to proceed.Aug 14 2018, 8:47 PM

Thanks. This is looking almost good, just the error handling for the close() and we should be done. I haven't been able to do run-time testing yet.
Again, the downloaded diff does not apply - what's wrong here?

--------------------------
|Index: security/ca_root_nss/files/MAca-bundle.pl.in
|===================================================================
|--- security/ca_root_nss/files/MAca-bundle.pl.in
|+++ security/ca_root_nss/files/MAca-bundle.pl.in
--------------------------
Patching file files/MAca-bundle.pl.in using Plan A...
Hunk #1 succeeded at 1.
Hunk #2 failed at 36.
Hunk #3 succeeded at 92.
Hunk #4 succeeded at 121.
Hunk #5 succeeded at 188.
1 out of 5 hunks failed while patching files/MAca-bundle.pl.in
done

I am not sure why the diff is failing for you.

I wonder if it is:

my $VERSION = '$FreeBSD$';

Whereas in your source file it'll be the SVN ID of the file

security/ca_root_nss/files/MAca-bundle.pl.in
256

I don't think close() can actually fail, but I'll add the error handling anyway

Im no perl fan so I wont comment on that aspect.

security/ca_root_nss/files/MAca-bundle.pl.in
9

Please move this line down one, your splitting an existing copyright in the middle. Yet another reason that "The all rights reserved" just needs to die die die

allanjude marked 11 inline comments as done.Aug 22 2018, 2:43 PM
allanjude updated this revision to Diff 47100.Aug 22 2018, 2:43 PM

Update with latest feedback

allanjude updated this revision to Diff 47101.Aug 22 2018, 2:47 PM

Fix a minor error

allanjude updated this revision to Diff 47103.Aug 22 2018, 2:53 PM

Fix comment style