[
https://issues.apache.org/jira/browse/YARN-8448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16648614#comment-16648614
]
Robert Kanter commented on YARN-8448:
-------------------------------------
Thanks for the feedback {{haibochen}}! Some comments:
{quote}In KeyStoreTestUtil.bytesToKeyStore(), should we use try clause for the
inputstream?
{quote}
This isn't actually neceessary because the inputstream is a
{{ByteArrayInputStream}}. It's read methods don't throw {{IOException}} because
it's not doing any real IO (so there's nothing to handle here), and it's
{{close}} method is empty (so it does nothing).
{quote}testLaunchContainerCopyFiles(boolean https) has a lot of if-statements
which I think justified having two different methods, each calling some utility
methods. Can you try to break it into two? Likewise for
testContainerLaunch(boolean https).
{quote}
I think trying to split these out into utility methods will actually be harder
to follow. While there are a number of if statements checking if it's using
HTTPS or not, each check only does a small thing. For instance, in
{{testLaunchContainerCopyFiles}}, the only real difference is that whether or
not we have the keystore and truststore, and so there's an if statement to
write those files, to add them to the {{ContainerStartContext}}, and to check
that they exist - the rest of the test is identical.
{quote}In the host verifier, does the peer certificates come in any order?
Right now the code assumes that the 1st one is always signed by the ca cert.
{quote}
I can't find any docs on the ordering, but it shouldn't matter anyways because
both certs are signed with the same key (the CA key). You can see that we use
the CA's public key to verify both certs in the custom {{X509TrustManager}}.
The only reason we also verify (one of the) certs in the custom
{{HostnameVerifier}} is because we need to determine if we should ignore the
hostname of the certificate, or if we should fallback to the default one, which
does check the hostname; this was a convinent way to check if it's one of our
certs vs a real cert.
{quote}KeyPairGenerator is created locally. Is there a security reason not to
reuse KeyPairGenerator?
{quote}
>From what I can tell, there's no security issue with reusing a
>{{KeyPairGenerator}}, but it's unclear if it's thread safe; so it's safest to
>assume it isn't. That seems to be what people suggest (see
>[here|https://stackoverflow.com/questions/25691151/is-keypairgenerator-generatekeypair-thread-safe]
> and
>[here|http://bouncy-castle.1462172.n4.nabble.com/is-key-generation-thread-safe-td4658456.html])
{quote}In the custom X509TrustManager, how would the defaultTrustManager verify
the identify of the AM?
{quote}
If we determine that the cert was issued by the RM ({{issuedByRM==true}}), then
at the end of the method, we check that the Subject is "CN=<app_id>". That will
only match if the RM connected to the AM it thought it was connecting to.
The 009 patch:
- Rebased on the latest trunk
- Addressed the cc warning
- Moved the secret keys to a new class, {{AMSecretKeys}}, in the
{{hadoop-yarn-server-common}} module
- Updated the wording of the config property in {{YarnConfiguration}} and
{{yarn-default.xml}}
- Changed the default to NONE, as per our offline discussion. In summary, we
don't need to generate certificates in a default non-HTTPS environment. If the
user sets up HTTPS for Hadoop, they can also change the config to LENIENT or
STRICT to get the AM certificates.
- Moved {{KEYSTORE_FILE_LOCATION}}, {{KEYSTORE_PASSWORD}},
{{TRUSTSTORE_FILE_LOCATION}}, and {{TRUSTSTORE_PASSWORD}} to
{{ApplicationConstants}}, and added javadoc
- {{DefaultLinuxContainerRuntime}} and {{DockerLinuxContainerRuntime}} are now
more defensive about null-checking for _both_ the keystore and truststore (that
shouldn't happen, but it is safer to check both in case that changes in the
future for some reason)
- In the C code, updated {{get_container_keystore_file}} and
{{get_container_truststore_file}} to to say "am container keystore" and "am
container truststore"
- Put back the exit code to {{OUT_OF_MEMORY}} for the string concat; I had
misread this before
- Removed the unnecessary checks before freeing possible NULL pointers
- Renamed {{COULD_NOT_CREATE_KEYSTORE_FILE}} to
{{COULD_NOT_CREATE_KEYSTORE_COPY}} and {{COULD_NOT_CREATE_TRUSTSTORE_FILE}} to
{{COULD_NOT_CREATE_TRUSTSTORE_COPY}} because we're copying and it's more
consistent with {{COULD_NOT_CREATE_SCRIPT_COPY}}. Also renamed
{{COULD_NOT_CREATE_CREDENTIALS_FILE}} to {{COULD_NOT_CREATE_CREDENTIALS_COPY}}
for the same reason.
- Renamed {{logpath}} to {{container_log_path}} and {{logpathapp}} to
{{app_log_path}} in {{test_launch_container}}
- Added {{@VisibleForTesting}} to {{ProxyCA#getCaCert}} and
{{ProxyCA#getCaKeyPair}}
- Split up {{TestProxyCA#testCreateTrustManager}} and
{{TestProxyCA#testCreateHostnameVerifier}} into multiple smaller more specific
tests
> AM HTTPS Support
> ----------------
>
> Key: YARN-8448
> URL: https://issues.apache.org/jira/browse/YARN-8448
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Robert Kanter
> Assignee: Robert Kanter
> Priority: Major
> Attachments: YARN-8448.001.patch, YARN-8448.002.patch,
> YARN-8448.003.patch, YARN-8448.004.patch, YARN-8448.005.patch,
> YARN-8448.006.patch, YARN-8448.007.patch, YARN-8448.008.patch,
> YARN-8448.009.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
