1137437 - move security/apps/ cert headers to moz.build's GENERATED_FILES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

This is straightforward enough, but it's not obvious to me what to do with the generation of manifest-signing-root.inc.  I don't understand bug 1059208 comment 21 for the explanation of why trusted-app-public.der doesn't exist in the tree.  Is it reasonable to just create an empty trusted-app-public.der (or maybe stick "NOT A VALID CERT" in it), or should the generation of manifest-signing-root.inc be special-cased?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

My understanding is that Firefox OS partners will put their own public signing root in trust-app-public.der, and we (Mozilla) don't use it in general. I think as long as the build system generates a file with either the contents of a partner-specified root (i.e. that file) or just "const uint8_t trustedAppPublicRoot[] = { 0x0 };", that should be sufficient. So I think either your first or last suggestions should work.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: move security/apps/ cert header generation to moz.build

Moving the cert header generation to GENERATED_FILES means that we can
delete all the manually-written out rules; we can also delete the
export:: rule because the build system automatically builds
GENERATED_FILES during the export phase.  The only tricky bit is
generating manifest-signing-root.inc, but ideally the comments in
moz.build and gen_cert_header.py will be sufficient to explain what's
going on.

Asking keeler for review because this seems like a significant enough change
that some extra amount of oversight is required.

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 8570589 [details] [diff] [review]
move security/apps/ cert header generation to moz.build

>-manifest-signing-root.inc: $(TRUSTED_APP_PUBLIC) $(GEN_CERT_HEADER)
>-	$(PYTHON) $(GEN_CERT_HEADER) trustedAppPublicRoot $(TRUSTED_APP_PUBLIC) > $@
...
>+# Generating trustedAppPublicRoot is a little different, because we
>+# don't use it in Gecko, but we want to enable partners to drop in their
>+# own certs.  So we'll check for its existence here; if we declared the
>+# input in moz.build, the build system would complain that the relevant
>+# file isn't present.
>+TRUSTED_APP_PUBLIC_ROOT_CERT_FILE='trusted-app-public.der'
>+
>+def trustedAppPublicRoot(header):
>+  array_name = 'trustedAppPublicRoot'
>+
>+  if os.path.isfile(TRUSTED_APP_PUBLIC_ROOT_CERT_FILE):
>+    header.write(_create_header(array_name,
>+                                _file_byte_generator(TRUSTED_APP_PUBLIC_ROOT_CERT_FILE)))

It looks like we lose the dependency on trusted-app-public.der when it exists (so if I build with that file, then change it, the next build doesn't update manifest-signing-root.inc). Do we have a way of writing out a deps file from the script? Or does it have to be in the .inputs field of GENERATED_FILES?

Also, I noticed that if some of the inputs change, but the output (.inc file) doesn't, then make will try to rebuild the .inc file on every build. Is this an issue with GENERATED_FILES or just this python script in particular? STR:

1) Build
2) touch security/apps/gen_cert_header.py
3) cd obj/security/apps
4) run 'make' a few times (it should build things the first time, but not on the second or subsequent calls)

Everything else looks fine - just the usual 'make sucks'.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8570589 [details] [diff] [review]
> move security/apps/ cert header generation to moz.build
> 
> >-manifest-signing-root.inc: $(TRUSTED_APP_PUBLIC) $(GEN_CERT_HEADER)
> >-	$(PYTHON) $(GEN_CERT_HEADER) trustedAppPublicRoot $(TRUSTED_APP_PUBLIC) > $@
> ...
> >+# Generating trustedAppPublicRoot is a little different, because we
> >+# don't use it in Gecko, but we want to enable partners to drop in their
> >+# own certs.  So we'll check for its existence here; if we declared the
> >+# input in moz.build, the build system would complain that the relevant
> >+# file isn't present.
> >+TRUSTED_APP_PUBLIC_ROOT_CERT_FILE='trusted-app-public.der'
> >+
> >+def trustedAppPublicRoot(header):
> >+  array_name = 'trustedAppPublicRoot'
> >+
> >+  if os.path.isfile(TRUSTED_APP_PUBLIC_ROOT_CERT_FILE):
> >+    header.write(_create_header(array_name,
> >+                                _file_byte_generator(TRUSTED_APP_PUBLIC_ROOT_CERT_FILE)))
> 
> It looks like we lose the dependency on trusted-app-public.der when it
> exists (so if I build with that file, then change it, the next build doesn't
> update manifest-signing-root.inc). Do we have a way of writing out a deps
> file from the script? Or does it have to be in the .inputs field of
> GENERATED_FILES?

Well, if it's in the .inputs field, moz.build will complain about non-existent files.  I guess we could just create an empty file; comment 1 indicates that's OK, I think.

> Also, I noticed that if some of the inputs change, but the output (.inc
> file) doesn't, then make will try to rebuild the .inc file on every build.

Isn't this what one generally wants from a build system?  The .inc files are declared with a dependency on the script itself and the inputs, so of course make will try to rebuild them.

The header itself is written with FileAvoidWrite:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/file_generate.py#44

so we won't update the mtimes of the output file if there's no change, if that's really what you're asking.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> > Also, I noticed that if some of the inputs change, but the output (.inc
> > file) doesn't, then make will try to rebuild the .inc file on every build.
> 
> Isn't this what one generally wants from a build system?  The .inc files are
> declared with a dependency on the script itself and the inputs, so of course
> make will try to rebuild them.
> 
> The header itself is written with FileAvoidWrite:
> 
> http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> action/file_generate.py#44
> 
> so we won't update the mtimes of the output file if there's no change, if
> that's really what you're asking.

mshal and I looked at this on IRC.  If you merely |touch| the script or the inputs, then FileAvoidWrite won't rewrite the output file, which means that make thinks it's always out of date.

This is a problem, I think, but it's something to be worked around inside the build system, and not a problem with this patch.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8570589 [details] [diff] [review]
move security/apps/ cert header generation to moz.build

Review of attachment 8570589 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, as far as I can tell. I noted one nit.

::: security/apps/gen_cert_header.py
@@ +52,5 @@
> +    # Treat non-existent files the same as a file containing a lone 0;
> +    # an single-element array 0 will fail cert verification just as an
> +    # empty array would.
> +    header.write(_create_header(array_name, ['\0']))
> +  

nit: trailing whitespace (in fact, the trailing line can probably be removed entirely)

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: move security/apps/ cert header generation to moz.build

Moving the cert header generation to GENERATED_FILES means that we can
delete all the manually-written out rules; we can also delete the
export:: rule because the build system automatically builds
GENERATED_FILES during the export phase.  For ease of converion, we opt
to create an empty trusted-app-public.der cert for manifest-signing-root.inc;
partners are free to overwrite that cert with their own.

Re-asking for r+ from dkeeler to confirm the empty file approach is OK.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8586851 [details] [diff] [review]
move security/apps/ cert header generation to moz.build

OK, apparently this new-fangled approach to attaching patches needs some more work.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: move security/apps/ cert header generation to moz.build

Moving the cert header generation to GENERATED_FILES means that we can
delete all the manually-written out rules; we can also delete the
export:: rule because the build system automatically builds
GENERATED_FILES during the export phase.  For ease of converion, we opt
to create an empty trusted-app-public.der cert for manifest-signing-root.inc;
partners are free to overwrite that cert with their own.

Re-flagging keeler to ensure he's OK with the empty-file approach for the
trusted-app cert.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8591825 [details] [diff] [review]
move security/apps/ cert header generation to moz.build

Review of attachment 8591825 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8591825 [details] [diff] [review]
move security/apps/ cert header generation to moz.build

Looks good! I'm not sure how we can solve the FileAvoidWrite rebuilding issue, but that shouldn't block you here.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33c7e76b28d3