Closed Bug 1164714 Opened 4 years ago Closed 4 years ago

Flatten away public/src subdirectories under security/manager/

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(8 files, 3 obsolete files)

9.77 KB, patch
keeler
: review+
Details | Diff | Splinter Review
31.07 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.94 KB, patch
keeler
: review+
Details | Diff | Splinter Review
3.66 KB, patch
keeler
: review+
Details | Diff | Splinter Review
15.33 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.91 KB, patch
keeler
: review+
mcmanus
: review+
Details | Diff | Splinter Review
7.46 KB, patch
keeler
: review+
coop
: checkin+
Details | Diff | Splinter Review
1.56 KB, patch
keeler
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8605575 - Flags: review?(dkeeler)
Attachment #8605576 - Flags: review?(dkeeler)
Is this part of a larger effort to flatten the source tree? I'm not against doing this, but what is the expected benefit? In particular, since this complicates the (admittedly already complicated) source history, we should make sure we're getting the most out of it. For example, it might be useful to go even further and merge boot and ssl, since I don't think there's a meaningful distinction between the two. Also, I think we should take the opportunity to move netwerk/base/nsISiteSecurityService.idl to be closer to the implementation (nsSiteSecurityService.cpp).
Flags: needinfo?(birunthan)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> Is this part of a larger effort to flatten the source tree? I'm not against
> doing this, but what is the expected benefit?

Yep. There isn't a meta bug, but this is similar to e.g. bug 1058101 and bug 1028559. The only benefits are a flatter tree and consistency (most public/src directories have been already flattened).

If you want to go ahead with this, I'll incorporate the suggestions in comment 7 into the patches.
Flags: needinfo?(birunthan)
Ok - this sounds like a good thing to do. I'll wait for the updated patches to review the whole batch. Also, to be clear, my thought was that everything in boot would be moved to ssl, which can keep the same name for now. Eventually manager/pki should be moved elsewhere (to toolkit, for example) and we can flatten manager/ssl into just manager or maybe "psm".
Attachment #8605575 - Attachment is obsolete: true
Attachment #8605576 - Attachment is obsolete: true
Attachment #8605575 - Flags: review?(dkeeler)
Attachment #8605576 - Flags: review?(dkeeler)
Attachment #8606036 - Flags: review?(dkeeler)
Comment on attachment 8605574 [details] [diff] [review]
Flatten security/manager/ssl/src/ directory

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

::: toolkit/devtools/webconsole/network-helper.js
@@ +558,5 @@
>       *      => state === "insecure"
>       *
>       * - request is HTTPS but it uses a weak cipher or old protocol, see
>       *   http://hg.mozilla.org/mozilla-central/annotate/def6ed9d1c1a/
> +     *   security/manager/ssl/nsNSSCallbacks.cpp#l1233

Since this refers to a specific revision, I don't think it works to change just the path. It would probably be best to change this comment in a later commit that references the new location.

@@ +731,5 @@
>      const wpl = Ci.nsIWebProgressListener;
>  
>      // If there's non-fatal security issues the request has STATE_IS_BROKEN
>      // flag set. See http://hg.mozilla.org/mozilla-central/file/44344099d119
> +    // /security/manager/ssl/nsNSSCallbacks.cpp#l1233

Same here.
Attachment #8605574 - Flags: review?(dkeeler) → review+
I just realized this requires coordination with the automation team that handles periodic scripts that update in-tree data structures. The HSTS and key pinning preload lists are updated by an external process that I think will break when we change the location of the output files. need-infoing :coop for help with that.
Flags: needinfo?(coop)
Comment on attachment 8606036 [details] [diff] [review]
Move and flatten security/manager/boot/{public,src}/ into security/manager/ssl/

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

r=me given we properly handle updating the automatic update scripts I mentioned in comment 13 (I imagine we'll have to open another bug for that).

::: security/manager/ssl/moz.build
@@ +78,3 @@
>      'CryptoTask.cpp',
> +    'DataStorage.cpp',
> +    'nsBOOTModule.cpp',

Hmmm - is having two different modules in the same directory going to cause a problem? Maybe we should actually merge the two (the other being nsNSSModule.cpp).
Attachment #8606036 - Flags: review?(dkeeler) → review+
Comment on attachment 8606038 [details] [diff] [review]
Move netwerk/base/nsISiteSecurityService.idl into security/manager/ssl

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

I think this is the right thing to do, but I think Patrick should sign off on it as well.
Attachment #8606038 - Flags: review?(mcmanus)
Attachment #8606038 - Flags: review?(dkeeler)
Attachment #8606038 - Flags: review+
I'd also like feedback from Honza and Cykesiopka that this is a reasonable approach.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(cykesiopka.bmo)
Comment on attachment 8606038 [details] [diff] [review]
Move netwerk/base/nsISiteSecurityService.idl into security/manager/ssl

there is also a netwerk/tests/TestSTSParser.cpp that you should probably take with it
Attachment #8606038 - Flags: review?(mcmanus) → review+
The general approach looks fine to me.
Flags: needinfo?(cykesiopka.bmo)
The patches are correctly doing a rename and the move makes sense to me.
r+
Flags: needinfo?(honzab.moz)
Landed the pki/ patches. I'll get the preload lists sorted out before landing the other bits.
Keywords: leave-open
Since everything seems to be moving into security/manager/ssl/, this patch should allow the periodic files updates to continue after flattening occurs. Pegging keeler for review to verify that the nsSTSPreloadList.* and StaticHPKPins.* will in fact be moving there.

The periodic update script runs every weekend, so this patch doesn't need to land immediately with the other changes, but it will need to land within the same week or in-tree HSTS/HPKP updates will fail until we do.

What is the uplift story likely to be here? Will it ride the trains normally? If so, we may need to use a tagged version of the script on release branches until regular uplift happens.
Attachment #8610179 - Flags: review?(dkeeler)
Comment on attachment 8610179 [details] [diff] [review]
[tools] Update script and output paths for periodically updated security files (HSTS and HPKP)

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

This looks correct to me.
I don't know what the uplift story will be. I imagine the safest bet would be to let this ride the trains, but that will make any other patches we have to uplift a bit more complicated (then again, it really should just be a search/replace in the patch files).
Attachment #8610179 - Flags: review?(dkeeler) → review+
I went ahead and landed this. Chris, could you land the periodic_file_updates.sh patch and switch to tagged versions for release branches?

(In reply to PTO - Patrick McManus [:mcmanus] from comment #18)
> there is also a netwerk/tests/TestSTSParser.cpp that you should probably
> take with it

I'll leave this for a separate patch. Didn't want to jinx a green try run.
Flags: needinfo?(coop)
Blocks: 1169399
(In reply to Birunthan Mohanathas [:poiru] from comment #27)
> I went ahead and landed this. Chris, could you land the
> periodic_file_updates.sh patch and switch to tagged versions for release
> branches?

That's a separate patch I'll need to write for the automation. I'll take a stab at the patch tomorrow, but branches other than m-c may not get periodic file updates this week if I can't deploy it before the weekend.
Flags: needinfo?(coop)
Blocks: 1169686
Updated the patch to work on both flattened and unflattened branches, based on the output of the new is_flattened function.

I looked into pulling the script by revision, but the buildbot factories don't make this easy. I'd need to make coordinated changes in multiple repos.
Attachment #8610179 - Attachment is obsolete: true
Attachment #8613012 - Flags: review?(dkeeler)
Comment on attachment 8613012 [details] [diff] [review]
[tools] Update script and output paths for periodically updated security files (HSTS and HPKP), v2

I should also note that I've tested this successfully in dry-run mode against both m-c and aurora, so both paths work.
Comment on attachment 8613012 [details] [diff] [review]
[tools] Update script and output paths for periodically updated security files (HSTS and HPKP), v2

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

This looks great - thanks!
Attachment #8613012 - Flags: review?(dkeeler) → review+
Comment on attachment 8613012 [details] [diff] [review]
[tools] Update script and output paths for periodically updated security files (HSTS and HPKP), v2

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

https://hg.mozilla.org/build/tools/rev/4f58767f30b0
Attachment #8613012 - Flags: checkin+
Comment on attachment 8621245 [details] [diff] [review]
Move netwerk/test/TestSTSParser.cpp into security/manager/ssl/tests/

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

LGTM.
Attachment #8621245 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/4c59a5c5da02
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.