Copy some x509_certificate_model_nss functions to src/chromeos

Copy some x509_certificate_model_nss functions to src/chromeos

Currently the Chrome OS network configuration UI is in src/chrome.

We want to move this to Settings UI which uses the networkingPrivate
extension API which is implemented in src/chromeos.

For network configuration we need to provide a list of available
certificates, including their display names, so we will need to
duplicate much of the logic in CertLibrary.

This is some initial cleanup work which removes the CertLibrary
dependency on chrome/common/net/x509_certificate_model_nss.cc
(which in turn depends on chrome/third_party/mozilla_security_manager
and contains significantly more code than CertLibrary needs and
would be difficult to migrate out of src/chrome).

It also removes some unnecessary src/chrome dependencies, and modifies
onc_certificate_importer_impl_unittest.cc to use the new helper method
instead of its own copied version of GetCertType.

BUG=380937
Review-Url: https://codereview.chromium.org/2871993005
Cr-Commit-Position: refs/heads/master@{#472488}
Committed: https://chromium.googlesource.com/chromium/src/+/cacb41c8337950175840b754a368cec5c9632b8a

[stevenjb, 2017-05-09 22:59:17]

Description was changed from

==========
Copy some x509_certificate_model_nss functions to src/chromeos

Currently the Chrome OS network configuration UI is in src/chrome.

We want to move this to Settings UI which uses the networkingPrivate
extension API which is implemented in src/chromeos.

For network configuration we need to provide a list of available
certificates, including their display names, so we will need to
duplicate much of the logic in CertLibrary.

This is some initial cleanup work which removes the CertLibrary
dependency on chrome/common/net/x509_certificate_model_nss.cc
(which in turn depends on chrome/third_party/mozilla_security_manager
and contains significantly more code than CertLibrary needs and
would be difficult to migrate out of src/chrome).

It also removes some unnecessary src/chrome dependencies.

BUG=380937
==========

to

==========
Copy some x509_certificate_model_nss functions to src/chromeos

Currently the Chrome OS network configuration UI is in src/chrome.

We want to move this to Settings UI which uses the networkingPrivate
extension API which is implemented in src/chromeos.

For network configuration we need to provide a list of available
certificates, including their display names, so we will need to
duplicate much of the logic in CertLibrary.

This is some initial cleanup work which removes the CertLibrary
dependency on chrome/common/net/x509_certificate_model_nss.cc
(which in turn depends on chrome/third_party/mozilla_security_manager
and contains significantly more code than CertLibrary needs and
would be difficult to migrate out of src/chrome).

It also removes some unnecessary src/chrome dependencies, and modifies
onc_certificate_importer_impl_unittest.cc to use the new helper method
instead of its own copied version of GetCertType.

BUG=380937
==========

[stevenjb, 2017-05-09 22:59:54]

stevenjb@chromium.org changed reviewers:
+ mattm@chromium.org, pmarko@chromium.org, tbarzic@chromium.org

[pmarko, 2017-05-11 12:32:46]

LGTM with nits/questions, but I'm not an OWNER of anything :)

I know this CL is explicitly copying things, but would it still be possible to
move this to a shared place and use it from both sides (so
x509_certificate_model_nss.cc could actually use the same code)? Or is that a
non-goal?

Also, I've seen that you've removed the macro definition supporting older NSS
versions in GetCertType. Is this fine because we're not supporting those older
versions anymore?

Thanks!

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.cc (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:22: // memory.  If the arg is null, an
empty string will be returned instead.
- Please remove extra space
- I'd suggest |nss_text| instead of "the arg"
- the second part of the sentence seems to be false: |alternative_text| is
returned instead.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:93:
Stringize(CERT_GetCommonName(&cert_handle->subject), std::string());
Couldn't we pass GetNickname(cert_handle) here instead of doing the if thing
below? Or is this an optimization?

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.h (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:24: std::string
GetCertTokenName(CERTCertificate* cert);
This is the only one taking CERTCertificate* instead of OSCertHandle, but I see
that it gets the same thing passed in at the callsite as GetCertType. Is that
fine?

[tbarzic, 2017-05-11 19:48:32]

lgtm

though, it would be nice if we could avoid having two copies of the same code.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.h (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:32: // or |cert_handle|->nickname if it is
unavailable or empty.
nit: s/it/the subject name/

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:34: // this does not include the raw
unformatted version.
wdyt about:
Unlike x509_certificate_model::GetCertNameOrNickname in src/chrome,
the raw unformated name or nickmane will not be included in the returned value.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper_unittest.cc (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper_unittest.cc:7: #include <stddef.h>
cstddef?

btw. why do you need this?

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper_unittest.cc:9: #include
"base/files/file_path.h"
this also seems unused

[tbarzic, 2017-05-11 19:48:33]

lgtm

though, it would be nice if we could avoid having two copies of the same code.

[stevenjb, 2017-05-12 19:27:49]

stevenjb@chromium.org changed reviewers:
+ emaxx@chromium.org

[stevenjb, 2017-05-12 19:27:50]

To answer the top level questions:
1. I don't think Chrome OS ever used the older version of NSS, and I am pretty
certain we do not now (but will find out soon enough if I am wrong...). The code
this was copied from may be called from a much broader range of platforms.

2. Unfortunately there is not a great common place for this code. The best
candidate would be src/net, but I ran it to a great deal of resistance the last
time I tried to move some of this there. Given that this is just a relatively
small amount of code, and given that it is really just glue code (i.e. it
doesn't do anything complicated), maintaining it separately just from chormeos
seems like the best solution to me.

+emaxx@ for another pair of eyes.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.cc (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:22: // memory.  If the arg is null, an
empty string will be returned instead.
On 2017/05/11 12:32:46, pmarko wrote:
> - Please remove extra space
> - I'd suggest |nss_text| instead of "the arg"
> - the second part of the sentence seems to be false: |alternative_text| is
> returned instead.

Done.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:93:
Stringize(CERT_GetCommonName(&cert_handle->subject), std::string());
On 2017/05/11 12:32:46, pmarko wrote:
> Couldn't we pass GetNickname(cert_handle) here instead of doing the if thing
> below? Or is this an optimization?

This was copied, but I think the extra simplicity is worth a small amount of
extra overhead so done (here and above).

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.h (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:24: std::string
GetCertTokenName(CERTCertificate* cert);
On 2017/05/11 12:32:46, pmarko wrote:
> This is the only one taking CERTCertificate* instead of OSCertHandle, but I
see
> that it gets the same thing passed in at the callsite as GetCertType. Is that
> fine?

That's a good catch. I copied this one from nsNSSCertificate.h which is why it
is different.

X509Certificate::OSCertHandle is typedef'd to 'struct CERTCertificateStr' in
x509_certificate.h, and CERTCertificateStr is typedefid to CERTCertificate in
x509_util_nss.h, so that is why this works.

I will change this to use net::X509Certificate::OSCertHandle instead for
consistency.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:32: // or |cert_handle|->nickname if it is
unavailable or empty.
On 2017/05/11 19:48:31, tbarzic wrote:
> nit: s/it/the subject name/

Done.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:34: // this does not include the raw
unformatted version.
On 2017/05/11 19:48:31, tbarzic wrote:
> wdyt about:
> Unlike x509_certificate_model::GetCertNameOrNickname in src/chrome,
> the raw unformated name or nickmane will not be included in the returned
value.

Done.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper_unittest.cc (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper_unittest.cc:7: #include <stddef.h>
On 2017/05/11 19:48:31, tbarzic wrote:
> cstddef?
> 
> btw. why do you need this?

Done.

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper_unittest.cc:9: #include
"base/files/file_path.h"
On 2017/05/11 19:48:31, tbarzic wrote:
> this also seems unused

Done.

[stevenjb, 2017-05-12 19:28:02]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 19:28:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 21:25:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 21:25:13]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-12 22:27:54]

Could you clarify a few questions regarding the CL description (and motivation)?

The networkingPrivate appears to be implemented under chrome/ and extensions/,
not under chromeos.

Current configuration UI = ? (views?)

Does "Settings UI" = webUI?  (eg, chrome://settings?) 
If so, is there some background on why it uses an extensions API instead of the
regular webUI c++ bindings?

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
File chromeos/network/certificate_helper.h (right):

https://codereview.chromium.org/2871993005/diff/20001/chromeos/network/certif...
chromeos/network/certificate_helper.h:24: std::string
GetCertTokenName(CERTCertificate* cert);
On 2017/05/12 19:27:50, stevenjb wrote:
> On 2017/05/11 12:32:46, pmarko wrote:
> > This is the only one taking CERTCertificate* instead of OSCertHandle, but I
> see
> > that it gets the same thing passed in at the callsite as GetCertType. Is
that
> > fine?
> 
> That's a good catch. I copied this one from nsNSSCertificate.h which is why it
> is different.
> 
> X509Certificate::OSCertHandle is typedef'd to 'struct CERTCertificateStr' in
> x509_certificate.h, and CERTCertificateStr is typedefid to CERTCertificate in
> x509_util_nss.h, so that is why this works.
> 
> I will change this to use net::X509Certificate::OSCertHandle instead for
> consistency.

Just fyi, in the future OSCertHandle will not be a CERTCertificate (see
crbug.com/671420). So you could make things that will be a CERTCertificate
explicitly say that. Or not. There's tons of stuff that will need to be changed,
so don't worry about it if you don't want to.

[stevenjb, 2017-05-12 22:39:48]

On Fri, May 12, 2017 at 4:27 PM, <mattm@chromium.org> wrote:

> Could you clarify a few questions regarding the CL description (and
> motivation)?
>
> The networkingPrivate appears to be implemented under chrome/ and
> extensions/,
> not under chromeos.
>
> networkingPrivate is implemented fully in src/extensions and src/chromeos,
with the exception of a delegate for some deprecated Cast functionality.
This is intentional so that we can handle network configuration without
chrome needing to run. Currently it can be run from a kiosk app, in the
future we plan to implement the networkingPrivate API as a mojo API, hosted
in either ash or a system services binary, but not chrome.



> Current configuration UI = ? (views?)
>
> Current configuration UI is Views based in Chrone, e.g.
wifi_config_view.cc.


> Does "Settings UI" = webUI? (eg, chrome://settings?)
>
Correct.


> If so, is there some background on why it uses an extensions API instead
> of the
> regular webUI c++ bindings?
>

a) so that it can be used by kiosk apps
b) extension APIs are well defined with type checking, whereas webUI c++
bindings use chrome.send which relies on an ad-hoc API.


>
>
>
> https://codereview.chromium.org/2871993005/diff/20001/
> chromeos/network/certificate_helper.h
> File chromeos/network/certificate_helper.h (right):
>
> https://codereview.chromium.org/2871993005/diff/20001/
> chromeos/network/certificate_helper.h#newcode24
> chromeos/network/certificate_helper.h:24: std::string
> GetCertTokenName(CERTCertificate* cert);
> On 2017/05/12 19:27:50, stevenjb wrote:
> > On 2017/05/11 12:32:46, pmarko wrote:
> > > This is the only one taking CERTCertificate* instead of
> OSCertHandle, but I
> > see
> > > that it gets the same thing passed in at the callsite as
> GetCertType. Is that
> > > fine?
> >
> > That's a good catch. I copied this one from nsNSSCertificate.h which
> is why it
> > is different.
> >
> > X509Certificate::OSCertHandle is typedef'd to 'struct
> CERTCertificateStr' in
> > x509_certificate.h, and CERTCertificateStr is typedefid to
> CERTCertificate in
> > x509_util_nss.h, so that is why this works.
> >
> > I will change this to use net::X509Certificate::OSCertHandle instead
> for
> > consistency.
>
> Just fyi, in the future OSCertHandle will not be a CERTCertificate (see
> crbug.com/671420). So you could make things that will be a
> CERTCertificate explicitly say that. Or not. There's tons of stuff that
> will need to be changed, so don't worry about it if you don't want to.
>

Thanks for the heads up. I am not nearly as up to speed on certificate
management as I would like to be. I will be more than happy to help
transition this code to something that lives in net/ or components/ or
anywhere that is not chrome/ or ui/ so that the network configuration layer
can use it without any chrome or ui dependencies. I am just not
knowledgeable enough to push that sort of change forward myself.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[emaxx, 2017-05-15 14:14:20]

LGTM with a note.

https://codereview.chromium.org/2871993005/diff/40001/chromeos/network/certif...
File chromeos/network/certificate_helper.cc (right):

https://codereview.chromium.org/2871993005/diff/40001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:89: return
Stringize(CERT_GetCommonName(&cert_handle->subject), alternative_text);
If CERT_GetCommonName() returns a non-null string that becomes empty after
stringizing, then this function will also return an empty string. Meanwhile the
x509_certificate_model analog seems to return the nickname in this case.
Is this an expected difference?

[stevenjb, 2017-05-15 15:40:22]

Patchset #4 (id:60001) has been deleted

[stevenjb, 2017-05-15 15:40:58]

mattm@, PTAL

https://codereview.chromium.org/2871993005/diff/40001/chromeos/network/certif...
File chromeos/network/certificate_helper.cc (right):

https://codereview.chromium.org/2871993005/diff/40001/chromeos/network/certif...
chromeos/network/certificate_helper.cc:89: return
Stringize(CERT_GetCommonName(&cert_handle->subject), alternative_text);
On 2017/05/15 14:14:20, emaxx wrote:
> If CERT_GetCommonName() returns a non-null string that becomes empty after
> stringizing, then this function will also return an empty string. Meanwhile
the
> x509_certificate_model analog seems to return the nickname in this case.
> Is this an expected difference?

Stringize() just constructs a string() from a char* and frees the char*, so this
could only happen if the char* points to an empty string, which doesn't seem
likelikely, but I'll modify Stringize() to handle it.

Done.

[stevenjb, 2017-05-16 16:39:15]

mattm@ - ping

[mattm, 2017-05-17 01:14:55]

lgtm

[stevenjb, 2017-05-17 15:54:25]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-05-17 15:54:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pmarko@chromium.org,
tbarzic@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2871993005/#ps80001
(title: "Handle empty string in Stringize, use OSCertHandle throughout")

[commit-bot: I haz the power, 2017-05-17 15:54:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:24:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495036465347050,
"parent_rev": "3f2e3a76424261598c4033dcb06e49f9c3205808", "commit_rev":
"cacb41c8337950175840b754a368cec5c9632b8a"}

[commit-bot: I haz the power, 2017-05-17 17:24:43]

Description was changed from

==========
Copy some x509_certificate_model_nss functions to src/chromeos

Currently the Chrome OS network configuration UI is in src/chrome.

We want to move this to Settings UI which uses the networkingPrivate
extension API which is implemented in src/chromeos.

For network configuration we need to provide a list of available
certificates, including their display names, so we will need to
duplicate much of the logic in CertLibrary.

This is some initial cleanup work which removes the CertLibrary
dependency on chrome/common/net/x509_certificate_model_nss.cc
(which in turn depends on chrome/third_party/mozilla_security_manager
and contains significantly more code than CertLibrary needs and
would be difficult to migrate out of src/chrome).

It also removes some unnecessary src/chrome dependencies, and modifies
onc_certificate_importer_impl_unittest.cc to use the new helper method
instead of its own copied version of GetCertType.

BUG=380937
==========

to

==========
Copy some x509_certificate_model_nss functions to src/chromeos

Currently the Chrome OS network configuration UI is in src/chrome.

We want to move this to Settings UI which uses the networkingPrivate
extension API which is implemented in src/chromeos.

For network configuration we need to provide a list of available
certificates, including their display names, so we will need to
duplicate much of the logic in CertLibrary.

This is some initial cleanup work which removes the CertLibrary
dependency on chrome/common/net/x509_certificate_model_nss.cc
(which in turn depends on chrome/third_party/mozilla_security_manager
and contains significantly more code than CertLibrary needs and
would be difficult to migrate out of src/chrome).

It also removes some unnecessary src/chrome dependencies, and modifies
onc_certificate_importer_impl_unittest.cc to use the new helper method
instead of its own copied version of GetCertType.

BUG=380937

Review-Url: https://codereview.chromium.org/2871993005
Cr-Commit-Position: refs/heads/master@{#472488}
Committed:
https://chromium.googlesource.com/chromium/src/+/cacb41c8337950175840b754a368...
==========

[commit-bot: I haz the power, 2017-05-17 17:24:45]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cacb41c8337950175840b754a368...

[afakhry, 2017-05-17 17:54:56]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2886213002/ by afakhry@chromium.org.

The reason for reverting is: Breaks the Linux ChromiumOS Builder (dbg) 
Failure to link:
FAILED: chromeos_unittests 
python "../../build/toolchain/gcc_link_wrapper.py"
--output="./chromeos_unittests" --
../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--no-as-needed
-lpthread -Wl,--as-needed -fuse-ld=gold
-B../../third_party/binutils/Linux_x64/Release/bin -Wl,--threads
-Wl,--thread-count=4 -Wl,--icf=all -m64 -pthread -Werror -Wl,--gdb-index
--sysroot=../../build/linux/debian_jessie_amd64-sysroot
-L/b/c/b/linux_chromeos/src/build/linux/debian_jessie_amd64-sysroot/lib/x86_64-linux-gnu
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/debian_jessie_amd64-sysroot/lib/x86_64-linux-gnu
-L/b/c/b/linux_chromeos/src/build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu
-Wl,-rpath-link=. -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/.
-Wl,-rpath-link=. -Wl,--export-dynamic -o "./chromeos_unittests"
-Wl,--start-group @"./chromeos_unittests.rsp" ./libonc.so ./libproxy_config.so
./libcrcrypto.so ./libnet.so ./liburl.so ./libprotobuf_lite.so ./libchromeos.so
./libdbus.so ./libbase.so ./libpolicy_proto.so ./libboringssl.so
./libbase_i18n.so ./libicui18n.so ./libicuuc.so ./libnet_with_v8.so
./libprefs.so -Wl,--end-group   -ldl -lrt -ldbus-1 -lnss3 -lnssutil3 -lsmime3
-lplds4 -lplc4 -lnspr4 -lgmodule-2.0 -lgobject-2.0 -lgthread-2.0 -lglib-2.0 
../../chromeos/network/certificate_helper_unittest.cc:19: error: undefined
reference to 'chromeos::certificate::GetCertNameOrNickname(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:25: error: undefined
reference to
'chromeos::certificate::GetCertAsciiNameOrNickname(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:27: error: undefined
reference to 'chromeos::certificate::GetCertNameOrNickname(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:34: error: undefined
reference to 'chromeos::certificate::GetCertNameOrNickname(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:43: error: undefined
reference to 'chromeos::certificate::GetCertType(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:56: error: undefined
reference to 'chromeos::certificate::GetCertType(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:68: error: undefined
reference to 'chromeos::certificate::GetCertType(CERTCertificateStr*)'
../../chromeos/network/certificate_helper_unittest.cc:80: error: undefined
reference to 'chromeos::certificate::GetCertType(CERTCertificateStr*)'.

[findit-for-me, 2017-05-17 18:10:15]

Findit (https://goo.gl/kROfz5) confirmed this CL at revision 472488 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...