joseluisll commented on code in PR #8703: URL: https://github.com/apache/hadoop/pull/8703#discussion_r4409593247
########## 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: Agreed. A null ServletContext now fails on the first `getAttribute`/`setAttribute` with an NPE that names the attribute. That is the same exception and the same point of failure as on trunk, where `ZKSignerSecretProvider.init` failed at `servletContext.getAttribute`. The map store is removed. ########## 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: The "implements neither" case is covered by `TestSignerSecretProviderCompatibility#testProviderImplementingNeitherMethodFailsLoudly`, which checks that the message names the provider class. The `initialize` javadoc says the default throws. ########## 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: `AuthenticationFilter` can't call `initialize` directly. A provider that overrides only `init` would then fall through to the default `initialize` and throw. That makes `init` the entry point every caller has to use until the namespace change, so I've removed the deprecation. The class javadoc still tells implementors to override `initialize`. ########## 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: Agreed, the null-cause branch can't run. Both sites now pass `ce.getCause()`. A package-private helper can't serve both sites: `CertificateUtil` is in `...authentication.util` and the handler is in `...authentication.server`. The helper would have to be a public method that names `ServletException`, on the class this change moves off the servlet API. A test pins the chain at each site (`TestCertificateUtil#testDeprecatedEntryPointDoesNotDeepenTheCauseChain`, `TestJWTRedirectAuthenticationHandler#testCorruptPublicKeyPEMCauseChain`), so if the two drift apart, a test fails. ########## 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: See the `CertificateUtil` thread. ########## 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: `SignerSecretProvider` is `@Private`/`@Unstable` on trunk, and `SecretProviderContext` has the same annotations as the class it is passed to. The `@deprecated` note on `init` has been removed (see the `init` thread), so there is no longer a deprecation note promising more than the annotations. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
