joseluisll commented on code in PR #8703: URL: https://github.com/apache/hadoop/pull/8703#discussion_r4409594663
########## 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: No longer applies: the map store has been removed. ########## 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: Fixed, both. ########## 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: Removed the stub. -- 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]
