[
https://issues.apache.org/jira/browse/HADOOP-19971?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133567#comment-18133567
]
ASF GitHub Bot commented on HADOOP-19971:
-----------------------------------------
pjfanning commented on code in PR #8703:
URL: https://github.com/apache/hadoop/pull/8703#discussion_r4400908502
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SignerSecretProvider.java:
##########
@@ -24,20 +24,77 @@
* do more complicated things in the backend.
* See the RolloverSignerSecretProvider class for an implementation that
* supports rolling over the secret at a regular interval.
+ * <p>
+ * Implementations should override
+ * {@link #initialize(Properties, SecretProviderContext, long)}, which names no
+ * servlet type. The older {@link #init(Properties, ServletContext, long)} is
+ * still the entry point callers use and is still honoured when an
+ * implementation overrides it, so providers written against it keep working
+ * unchanged; it is deprecated and will be removed with the move to the jakarta
+ * servlet namespace.
+ * <p>
+ * One consequence for code that reflects over these classes: the providers
+ * shipped here override
+ * {@link #initialize(Properties, SecretProviderContext, long)} rather than
+ * {@link #init(Properties, ServletContext, long)}, so init is no longer among
+ * their declared methods. Calls and overrides are unaffected - init is
+ * inherited from this class and resolves as it always did - but
+ * Class#getDeclaredMethod("init", ...) on a subclass such as
+ * {@link FileSignerSecretProvider} now raises NoSuchMethodException where it
+ * used to succeed. Class#getMethod, which searches superclasses, still finds
+ * it.
*/
@InterfaceStability.Unstable
@InterfaceAudience.Private
public abstract class SignerSecretProvider {
/**
- * Initialize the SignerSecretProvider
+ * Initialize the SignerSecretProvider.
+ * <p>
+ * The default implementation adapts the ServletContext to a
+ * {@link SecretProviderContext} and calls
+ * {@link #initialize(Properties, SecretProviderContext, long)}, so a
provider
+ * that overrides only that method is initialized correctly through this
+ * entry point. A provider that overrides this method instead is called
+ * directly, as before.
+ *
* @param config configuration properties
* @param servletContext servlet context
* @param tokenValidity The amount of time a token is valid for
* @throws Exception thrown if an error occurred
+ * @deprecated override
+ * {@link #initialize(Properties, SecretProviderContext, long)},
+ * which does not name a servlet type and so is unaffected by
+ * which servlet namespace the container provides.
*/
- public abstract void init(Properties config, ServletContext servletContext,
- long tokenValidity) throws Exception;
+ @Deprecated
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) throws Exception {
+ initialize(config, ServletSecretProviderContext.of(servletContext),
+ tokenValidity);
+ }
+
+ /**
+ * Initialize the SignerSecretProvider against an attribute store.
+ * <p>
+ * The default implementation throws, because a provider has to implement one
+ * of the two initialization methods. It is never reached by a provider that
+ * overrides the deprecated
+ * {@link #init(Properties, ServletContext, long)}, since callers go through
+ * that method and its override does not delegate here.
+ *
+ * @param config configuration properties
+ * @param context the attribute store to initialize against
+ * @param tokenValidity The amount of time a token is valid for
+ * @throws Exception thrown if an error occurred
+ */
+ public void initialize(Properties config, SecretProviderContext context,
Review Comment:
With `init` no longer abstract and `initialize` defaulting to a throw, the
compiler no longer forces a subclass to implement anything. A new provider that
implements neither method, or one that gets the `initialize` signature slightly
wrong without `@Override`, now compiles and fails with
`UnsupportedOperationException` at web-server startup instead of at build time.
I understand why (both entry points need a default), but it's worth noting in
the javadoc/release note, and a test covering the "implements neither" message
would pin the behaviour down.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/CertificateUtil.java:
##########
@@ -38,8 +38,37 @@ public class CertificateUtil {
* - the pem encoding from config without the header and footer
* @return RSAPublicKey the RSA public key
* @throws ServletException thrown if a processing error occurred
+ * @deprecated use {@link #toRSAPublicKey(String)}, which reports the
+ * {@link CertificateException} it is really raising instead of
+ * wrapping it in a servlet type. This method is kept so that
+ * existing callers still compile and behave as before, and will
+ * be removed with the move to the jakarta servlet namespace.
*/
- public static RSAPublicKey parseRSAPublicKey(String pem) throws
ServletException {
+ @Deprecated
+ public static RSAPublicKey parseRSAPublicKey(String pem)
+ throws ServletException {
+ try {
+ return toRSAPublicKey(pem);
+ } catch (CertificateException ce) {
+ // Report the exception toRSAPublicKey wrapped, not the wrapper, so that
+ // the cause a caller sees is the one this method has always reported:
+ // the CertificateException the parse itself raised. Wrapping the wrapper
+ // would add a level to the chain that was not there before.
+ Throwable cause = ce.getCause() == null ? ce : ce.getCause();
Review Comment:
`toRSAPublicKey` always throws a `CertificateException` with a non-null
cause, so the `ce.getCause() == null ? ce : ...` fallback can't run. The same
unwrap logic is also copied into `JWTRedirectAuthenticationHandler`. The
underlying issue is that `toRSAPublicKey` wraps a `CertificateException` in
another `CertificateException` only to change the message. A small
package-private helper (e.g. `static ServletException
toServletException(CertificateException)`) used by both call sites would keep
the cause chain in one place.
##########
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestSignerSecretProviderCompatibility.java:
##########
@@ -0,0 +1,228 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+
+import javax.servlet.ServletContext;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * A SignerSecretProvider written against the deprecated
+ * {@link SignerSecretProvider#init(Properties, ServletContext, long)} must
keep
+ * working without being recompiled, because implementations of it live outside
+ * this tree - signer.secret.provider takes a classname.
+ * <p>
+ * These tests pin that contract. They are the reason
+ * {@link SignerSecretProvider#init(Properties, ServletContext, long)} is a
+ * concrete bridge rather than a changed abstract method, and they go when it
+ * does.
+ */
+@SuppressWarnings("deprecation")
+public class TestSignerSecretProviderCompatibility {
+
+ private static final byte[] SECRET =
+ "secret".getBytes(StandardCharsets.UTF_8);
+
+ /**
+ * A provider as it would have been written before SecretProviderContext
+ * existed: it overrides init and takes a ServletContext.
+ */
+ private static final class LegacyProvider extends SignerSecretProvider {
+ private ServletContext seen;
+ private boolean initCalled;
+
+ @Override
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) {
+ this.seen = servletContext;
+ this.initCalled = true;
+ }
+
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ /**
+ * The same, extending RolloverSignerSecretProvider and chaining to super as
+ * such a provider is expected to.
+ */
+ private static final class LegacyRolloverProvider
+ extends RolloverSignerSecretProvider {
+ private boolean initCalled;
+
+ @Override
+ public void init(Properties config, ServletContext servletContext,
+ long tokenValidity) throws Exception {
+ this.initCalled = true;
+ super.init(config, servletContext, tokenValidity);
+ }
+
+ @Override
+ protected byte[] generateNewSecret() {
+ return SECRET;
+ }
+ }
+
+ /** A provider written against the servlet-free method. */
+ private static final class ContextProvider extends SignerSecretProvider {
+ private SecretProviderContext seen;
+
+ @Override
+ public void initialize(Properties config, SecretProviderContext context,
+ long tokenValidity) {
+ this.seen = context;
+ }
+
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ /** A provider that implements neither method. */
+ private static final class UninitializableProvider extends
SignerSecretProvider {
+ @Override
+ public byte[] getCurrentSecret() {
+ return SECRET;
+ }
+
+ @Override
+ public byte[][] getAllSecrets() {
+ return new byte[][]{SECRET};
+ }
+ }
+
+ @Test
+ public void testLegacyProviderStillReceivesTheServletContext()
+ throws Exception {
+ ServletContext servletContext = mock(ServletContext.class);
+ LegacyProvider provider = new LegacyProvider();
+
+ provider.init(new Properties(), servletContext, 1000);
+
+ assertTrue(provider.initCalled, "the legacy override should have run");
+ assertSame(servletContext, provider.seen);
+ }
+
+ @Test
+ public void testLegacyRolloverProviderIsStillRolledOver() throws Exception {
+ LegacyRolloverProvider provider = new LegacyRolloverProvider();
+ try {
+ provider.init(new Properties(), mock(ServletContext.class), 100000);
+
+ // The override ran, and chaining to super still reached
+ // RolloverSignerSecretProvider.initialize, which seeds the secret. Were
+ // the bridge dispatched statically, this would silently be null.
+ assertTrue(provider.initCalled, "the legacy override should have run");
+ assertArrayEquals(SECRET, provider.getCurrentSecret());
+ } finally {
+ provider.destroy();
+ }
+ }
+
+ @Test
+ public void testContextProviderIsReachedThroughTheDeprecatedEntryPoint()
+ throws Exception {
+ Map<String, Object> attributes = new HashMap<>();
+ ServletContext servletContext = mock(ServletContext.class);
+ when(servletContext.getAttribute("a"))
Review Comment:
Nit: this `getAttribute` stub reads from `attributes`, but nothing ever
writes to that map, and the test only verifies `setAttribute`. Either drop the
stub, or have a `setAttribute` answer populate the map and assert that
`provider.seen.getAttribute("a")` reads the value back through the servlet
context.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SecretProviderContext.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+
+/**
+ * The attribute store a {@link SignerSecretProvider} is initialized against.
+ * <p>
+ * A SignerSecretProvider that has to share an object with the rest of the
+ * application it is embedded in - as {@link ZKSignerSecretProvider} shares its
+ * CuratorFramework client - reads and writes that object here. When the
+ * provider is running inside a servlet container the attributes are those of
+ * the ServletContext, so the sharing is unchanged; see
+ * {@link
org.apache.hadoop.security.authentication.server.AuthenticationFilter}.
+ * <p>
+ * This interface exists so that a SignerSecretProvider need not name a servlet
+ * type. Implementations of it that are backed by a ServletContext do, but the
+ * providers themselves stay independent of the servlet API and so of which
+ * servlet namespace - javax or jakarta - the container provides.
+ */
[email protected]
[email protected]
Review Comment:
Minor: the `@deprecated` text on `SignerSecretProvider.init` tells
implementors to override `initialize`, which takes this `@Private`/`@Unstable`
type. `SignerSecretProvider` is itself `@Private`, so this is consistent. But
`signer.secret.provider` accepts a class name, so external providers do exist
in practice, and the deprecation is effectively advice for them. Consider
`@LimitedPrivate`/`@Evolving` for the pair, or wording the deprecation note so
it doesn't promise more than the annotations do.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/SignerSecretProvider.java:
##########
@@ -24,20 +24,77 @@
* do more complicated things in the backend.
* See the RolloverSignerSecretProvider class for an implementation that
* supports rolling over the secret at a regular interval.
+ * <p>
+ * Implementations should override
+ * {@link #initialize(Properties, SecretProviderContext, long)}, which names no
+ * servlet type. The older {@link #init(Properties, ServletContext, long)} is
+ * still the entry point callers use and is still honoured when an
+ * implementation overrides it, so providers written against it keep working
+ * unchanged; it is deprecated and will be removed with the move to the jakarta
+ * servlet namespace.
+ * <p>
+ * One consequence for code that reflects over these classes: the providers
+ * shipped here override
+ * {@link #initialize(Properties, SecretProviderContext, long)} rather than
+ * {@link #init(Properties, ServletContext, long)}, so init is no longer among
+ * their declared methods. Calls and overrides are unaffected - init is
+ * inherited from this class and resolves as it always did - but
+ * Class#getDeclaredMethod("init", ...) on a subclass such as
+ * {@link FileSignerSecretProvider} now raises NoSuchMethodException where it
+ * used to succeed. Class#getMethod, which searches superclasses, still finds
+ * it.
*/
@InterfaceStability.Unstable
@InterfaceAudience.Private
public abstract class SignerSecretProvider {
/**
- * Initialize the SignerSecretProvider
+ * Initialize the SignerSecretProvider.
+ * <p>
+ * The default implementation adapts the ServletContext to a
+ * {@link SecretProviderContext} and calls
+ * {@link #initialize(Properties, SecretProviderContext, long)}, so a
provider
+ * that overrides only that method is initialized correctly through this
+ * entry point. A provider that overrides this method instead is called
+ * directly, as before.
+ *
* @param config configuration properties
* @param servletContext servlet context
* @param tokenValidity The amount of time a token is valid for
* @throws Exception thrown if an error occurred
+ * @deprecated override
+ * {@link #initialize(Properties, SecretProviderContext, long)},
+ * which does not name a servlet type and so is unaffected by
+ * which servlet namespace the container provides.
*/
- public abstract void init(Properties config, ServletContext servletContext,
- long tokenValidity) throws Exception;
+ @Deprecated
+ public void init(Properties config, ServletContext servletContext,
Review Comment:
`init` is now `@Deprecated`, but every in-tree caller still uses it:
`AuthenticationFilter.constructSecretProvider` (5 calls) plus around 20 test
call sites. Each one now produces a new javac deprecation warning, which
Yetus's javac check is likely to flag against trunk. It also means nothing in
the tree calls `initialize` directly. Could `AuthenticationFilter` call
`initialize(config, ServletSecretProviderContext.of(ctx), validity)` for
providers that override `initialize`? Or, if the deprecation is mainly to steer
implementors, the deprecation could be deferred until the callers move.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/JWTRedirectAuthenticationHandler.java:
##########
@@ -125,7 +126,15 @@ public void init(Properties config) throws
ServletException {
throw new ServletException(
"Public key for signature validation must be provisioned.");
}
- publicKey = CertificateUtil.parseRSAPublicKey(pemPublicKey);
+ try {
+ publicKey = CertificateUtil.toRSAPublicKey(pemPublicKey);
+ } catch (CertificateException ce) {
+ // Report the exception toRSAPublicKey wrapped, not the wrapper, as
+ // CertificateUtil#parseRSAPublicKey does: the cause of this
+ // ServletException stays the CertificateException the parse raised.
+ Throwable cause = ce.getCause() == null ? ce : ce.getCause();
Review Comment:
This duplicates the unwrap-and-rewrap logic in
`CertificateUtil.parseRSAPublicKey`, including the null-cause branch that can't
happen. If the wrapping in `toRSAPublicKey` changes later, the two copies could
drift apart. See the comment on `CertificateUtil`: a shared helper would avoid
that.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ServletSecretProviderContext.java:
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.servlet.ServletContext;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.classification.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Presents a ServletContext to a {@link SignerSecretProvider} as a
+ * {@link SecretProviderContext}.
+ * <p>
+ * Reads and writes go straight through to the ServletContext, so an object a
+ * provider shares this way - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} - is still a ServletContext attribute under
+ * the same name, and is still found there by everything that looks for it,
+ * including DelegationTokenAuthenticationFilter in hadoop-common.
+ * <p>
+ * This class is the one place in the provider hierarchy that names a servlet
+ * type, and it goes away with the deprecated
+ * {@link SignerSecretProvider#init(java.util.Properties, ServletContext,
long)}
+ * it exists to support.
+ */
[email protected]
[email protected]
+final class ServletSecretProviderContext implements SecretProviderContext {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ServletSecretProviderContext.class);
+
+ private final ServletContext servletContext;
+
+ private ServletSecretProviderContext(ServletContext servletContext) {
+ this.servletContext = servletContext;
+ }
+
+ /**
+ * Returns a context backed by the given ServletContext, or one backed by a
+ * map of its own when there is no ServletContext.
+ * <p>
+ * Most callers of the deprecated init pass null - only AuthenticationFilter
+ * ever passes a real context - and a provider that does not use the store is
+ * unaffected either way. Handing back a store rather than null keeps a
+ * provider that does use it from having to check.
+ *
+ * @param servletContext the servlet context, or null
+ * @return an attribute store, never null
+ */
+ static SecretProviderContext of(ServletContext servletContext) {
+ return servletContext == null
+ ? new MapSecretProviderContext()
Review Comment:
I see the javadoc below calls this out deliberately, but it is a real
behaviour change for `ZKSignerSecretProvider`. On trunk, `init(config, null,
validity)` failed immediately (`servletContext.getAttribute` NPE). Now it
silently builds its own `CuratorFramework` client that nothing else can find
under `ZOOKEEPER_SIGNER_SECRET_PROVIDER_CURATOR_CLIENT_ATTRIBUTE`, so a
misconfigured caller gets an extra ZK session and possibly split secret/ACL
config, with only a WARN line.
Would it be safer to keep failing fast for a null context, e.g. have
`MapSecretProviderContext` (or `ZKSignerSecretProvider`) throw rather than warn
when asked to store the shared client? Providers that never touch the store
would be unaffected either way.
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/CertificateUtil.java:
##########
@@ -57,7 +86,7 @@ public static RSAPublicKey parseRSAPublicKey(String pem)
throws ServletException
} else {
message = "CertificateException - PEM may be corrupt";
}
- throw new ServletException(message, ce);
+ throw new CertificateException(message, ce);
}
return (RSAPublicKey) key;
Review Comment:
This was already the case, but since `toRSAPublicKey` is new API documented
as reporting parse failures via `CertificateException`: a valid non-RSA
certificate (EC/DSA) gets through `generateCertificate` and then throws an
unchecked `ClassCastException` here. Suggest:
```java
if (!(key instanceof RSAPublicKey)) {
throw new CertificateException("Public key is not an RSA key: " +
key.getAlgorithm());
}
return (RSAPublicKey) key;
```
##########
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestCertificateUtil.java:
##########
@@ -87,12 +89,62 @@ public void testValidPEM() throws Exception {
+
"9aTyR+HGHCfvwoCegc9rAVw/DLaRriSO/jnEXzYK6XLVKH+hx5UXrJ7Oyc7JjZUc3g9kCWORThCX"
+ "Mzc1xA==";
try {
- RSAPublicKey pk = CertificateUtil.parseRSAPublicKey(pem);
+ RSAPublicKey pk = CertificateUtil.toRSAPublicKey(pem);
assertNotNull(pk);
assertEquals("RSA", pk.getAlgorithm());
+ } catch (CertificateException se) {
+ fail("Should not have thrown CertificateException");
+ }
+ }
+
+ /**
+ * The deprecated entry point still reports a ServletException, so callers
+ * written against it keep compiling and keep catching what they caught
+ * before.
+ */
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedEntryPointStillThrowsServletException() {
+ String pem = "not a certificate";
+ try {
+ CertificateUtil.parseRSAPublicKey(pem);
+ fail("Should not have thrown ServletException");
Review Comment:
The failure message reads the wrong way round: this `fail` runs when
`ServletException` was *not* thrown, so it should be something like `"Should
have thrown ServletException"`. Same at line 128. (The "Should not have thrown
CertificateException" messages at 51/72/96 look inherited, but they're touched
by this PR, so they'd be worth fixing too.)
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ServletSecretProviderContext.java:
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. See accompanying LICENSE file.
+ */
+package org.apache.hadoop.security.authentication.util;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.servlet.ServletContext;
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.classification.VisibleForTesting;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Presents a ServletContext to a {@link SignerSecretProvider} as a
+ * {@link SecretProviderContext}.
+ * <p>
+ * Reads and writes go straight through to the ServletContext, so an object a
+ * provider shares this way - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} - is still a ServletContext attribute under
+ * the same name, and is still found there by everything that looks for it,
+ * including DelegationTokenAuthenticationFilter in hadoop-common.
+ * <p>
+ * This class is the one place in the provider hierarchy that names a servlet
+ * type, and it goes away with the deprecated
+ * {@link SignerSecretProvider#init(java.util.Properties, ServletContext,
long)}
+ * it exists to support.
+ */
[email protected]
[email protected]
+final class ServletSecretProviderContext implements SecretProviderContext {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ServletSecretProviderContext.class);
+
+ private final ServletContext servletContext;
+
+ private ServletSecretProviderContext(ServletContext servletContext) {
+ this.servletContext = servletContext;
+ }
+
+ /**
+ * Returns a context backed by the given ServletContext, or one backed by a
+ * map of its own when there is no ServletContext.
+ * <p>
+ * Most callers of the deprecated init pass null - only AuthenticationFilter
+ * ever passes a real context - and a provider that does not use the store is
+ * unaffected either way. Handing back a store rather than null keeps a
+ * provider that does use it from having to check.
+ *
+ * @param servletContext the servlet context, or null
+ * @return an attribute store, never null
+ */
+ static SecretProviderContext of(ServletContext servletContext) {
+ return servletContext == null
+ ? new MapSecretProviderContext()
+ : new ServletSecretProviderContext(servletContext);
+ }
+
+ @Override
+ public Object getAttribute(String name) {
+ return servletContext.getAttribute(name);
+ }
+
+ @Override
+ public void setAttribute(String name, Object value) {
+ servletContext.setAttribute(name, value);
+ }
+
+ /**
+ * The store used when there is no ServletContext to write through to.
+ * Attributes live as long as the provider does and are seen by nothing else,
+ * which is what passing a null ServletContext already meant.
+ */
+ @VisibleForTesting
+ static final class MapSecretProviderContext
+ implements SecretProviderContext {
+
+ private final Map<String, Object> attributes = new HashMap<>();
+
+ @Override
+ public Object getAttribute(String name) {
+ return attributes.get(name);
+ }
+
+ /**
+ * Stores the attribute, and says so.
+ * <p>
+ * A provider only writes here to share an object with the rest of the
+ * application - the CuratorFramework client of
+ * {@link ZKSignerSecretProvider} is the one case in this tree. There is
+ * nothing to share it with: this store is private to the provider, so a
+ * second provider in the same JVM builds a second client rather than
+ * finding this one. Before there was a store at all a null ServletContext
+ * threw, which was at least loud; this keeps it visible.
+ */
+ @Override
+ public void setAttribute(String name, Object value) {
+ LOG.warn("SignerSecretProvider was initialized without a ServletContext,"
+ + " so the attribute {} is being stored in a map private to this"
+ + " provider. Anything that expects to share it - including another"
+ + " provider in this JVM - will not find it there.", name);
+ attributes.put(name, value);
Review Comment:
Nit: `ServletContext.setAttribute(name, null)` removes the attribute,
whereas this stores a null value, so the two stores behave differently for a
provider that clears an attribute. Easy to match with `if (value == null)
attributes.remove(name); else attributes.put(name, value);`.
> Remove servlet and Jetty types from classes that do not need them
> -----------------------------------------------------------------
>
> Key: HADOOP-19971
> URL: https://issues.apache.org/jira/browse/HADOOP-19971
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: auth, common
> Reporter: Jose Luis López
> Assignee: Jose Luis López
> Priority: Minor
> Labels: pull-request-available
>
> h3. Why
> Hadoop will eventually move from the javax.servlet namespace to
> jakarta.servlet. When it does, every class that mentions a servlet type has
> to be revisited, and every class that mentions a Jetty type has to be
> revisited again when the Jetty API changes underneath it.
> Several of those classes have no real connection to either. A component that
> generates signing secrets was handed the web server's context even though it
> only wanted somewhere to store an object. A certificate parser reported a bad
> certificate as a web server error. A Netty-based shuffle handler borrowed two
> header names from Jetty, and the YARN services client borrowed Jetty's URL
> encoder.
> This change removes those references, so those classes drop out of the later
> migration entirely.
> h3. What changes for anyone using Hadoop
> Nothing is removed, no signature changes shape, and no dependency, version or
> setting changes. Code built against today's release keeps compiling and
> running without being rebuilt.
> * Two entry points get a servlet-free alternative. The old ones stay, marked
> deprecated, until the namespace change:
> ** {{SignerSecretProvider.init(Properties, ServletContext, long)}} ->
> {{initialize(Properties, SecretProviderContext, long)}}
> ** {{CertificateUtil.parseRSAPublicKey(String)}} -> {{toRSAPublicKey(String)}}
> * Providers that override {{init}} are still called directly. A provider that
> extends Hadoop's rolling secret provider and calls {{super.init}} still gets
> its rollover scheduler started. {{TestSignerSecretProviderCompatibility}}
> pins this contract.
> * Exceptions are unchanged: same type, message and cause chain from
> {{parseRSAPublicKey}} and from {{JWTRedirectAuthenticationHandler}} when the
> PEM is corrupt.
> * The shuffle response is unchanged, byte for byte: the same {{Connection}}
> and {{Keep-Alive}} header names, capitalised as before.
> * The YARN services client now encodes {{user.name}} with
> {{java.net.URLEncoder}}. That differs from Jetty's encoder for two characters
> only: {{*}} is now sent as is where it used to be escaped, and {{~}} is now
> escaped where it used to be sent as is. Every code point decodes to the same
> value either way (checked across all of Unicode against Jetty 9.4.58), so the
> server reads the same user name.
> Things a downstream project could notice:
> * Deprecation warnings where code overrides {{init}} or calls
> {{parseRSAPublicKey}}.
> * {{getDeclaredMethod("init", ...)}} on the shipped providers now throws
> {{NoSuchMethodException}}, because they declare {{initialize}} instead.
> {{getMethod}} still finds {{init}}.
> * {{ZKSignerSecretProvider}} given a null ServletContext used to throw a
> NullPointerException. It now keeps its ZooKeeper client in a store private to
> itself and logs a WARN. Every caller in Hadoop passes a real context, and
> there the client is still stored as the ServletContext attribute of the same
> name.
> h3. What changes inside
> || || before || after ||
> | modules using Jetty in main sources | 12 | 11 |
> | main-source files using Jetty | 26 | 24 |
> | hadoop-auth main classes naming a servlet | 14 | 11 |
> Counted on trunk at a7c2bea723d.
> The MapReduce shuffle module no longer uses Jetty at all. In hadoop-auth, the
> secret providers and the authentication token no longer mention the servlet
> API. They are initialised against a small two-method store,
> {{SecretProviderContext}}, which holds whatever needs sharing; that is all
> the one provider that used the servlet context ever did with it. One new
> package-private class mentions the servlet API, and it exists only to keep
> the old path working.
> h3. What this does not do
> No namespace change, no Jetty upgrade, no Jersey change, no EE environments.
> The tree stays on Jetty 9.4, Jersey 2 and javax.servlet.
> The authentication handler classes still mention the servlet API,
> deliberately. They are the extension point that HBase, Hive, Spark, Ozone and
> Knox build against, and changing it is exactly the kind of break this issue
> is meant to avoid. That belongs with the namespace change, where downstream
> projects will expect it.
> h3. Ordering
> This is preparation for the jakarta move. It depends on nothing, is based on
> trunk, and nothing in HADOOP-19972 depends on it. The plan is to land it with
> or just before the namespace change in HADOOP-19395, in the next major
> release.
> When rebasing onto a later trunk:
> * If HADOOP-19970 has landed, it declares jetty-http in the shuffle module's
> pom only because of the import this change removes. Drop that declaration
> here.
> * If HADOOP-19972 has landed, re-check the shuffle headers and the
> {{user.name}} encoding against whatever Jetty classes it left in those two
> places.
> 16 files changed. Works on Jetty 9.4 and on the Jetty 12 ee8 environment of
> HADOOP-19912, and commits the namespace change in HADOOP-19395 to neither
> ee10 nor ee11.
> Contains content generated by Claude.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
