mattcasters commented on code in PR #8793:
URL: https://github.com/apache/hop/pull/8793#discussion_r4229770636
##########
plugins/transforms/maskfields/src/main/java/org/apache/hop/pipeline/transforms/maskfields/MaskingRuntime.java:
##########
@@ -150,12 +155,14 @@ public void release() {
}
}
- private static String databaseKey(
- DatabaseMeta databaseMeta, String schemaName, String tableName) {
- String name = databaseMeta == null ? "" : databaseMeta.getName();
- String schema = schemaName == null ? "" : schemaName;
- String table = tableName == null ? "" : tableName;
- return name + "\0" + schema + "\0" + table;
+ static String databaseKey(
+ IVariables variables, DatabaseMeta databaseMeta, String schemaName,
String tableName)
+ throws HopException {
+ String url = Const.NVL(databaseMeta.getURL(variables), "");
Review Comment:
**[bug]** The store key is not the URL the connection actually opens.
`DatabaseMeta.getURL()` returns a manual JDBC URL unchanged;
`Database.connectUsingClass` only then does `resolve(getURL())`. Two pipelines
in one JVM that share a manual URL template such as
`jdbc:postgresql://${HOST}/${DB}` but resolve it to different databases still
get the same key and reuse whichever store connected first. Hostname and
database fields are substituted inside `getURL()`, so this is specific to
manual URLs, which is a common way to vary the target per environment. That is
the cross-database sharing this change is meant to stop.
**Suggestion:** Key on `variables.resolve(databaseMeta.getURL(variables))`,
the same string a non-tunnel `Database.connect` opens. If that string still
contains a variable token, do not share the store.
##########
plugins/transforms/maskfields/src/main/java/org/apache/hop/pipeline/transforms/maskfields/store/DatabaseMaskingStore.java:
##########
@@ -90,64 +97,103 @@ public void open() throws HopException {
database.setAutoCommit(false);
}
+ @Override
+ public String findOrCreate(String patternName, String sourceKey,
MaskAllocator allocator)
+ throws HopException {
+ return findOrCreate(patternName, sourceKey, null, allocator);
+ }
+
+ /**
+ * Finds or stores the replacement in one transaction. When another process
stores the same source
+ * key, the next sequence row or the same replacement at the same moment,
the constraint violation
+ * rolls the attempt back and the next attempt sees what that process
committed.
+ */
@Override
public synchronized String findOrCreate(
- String patternName, String sourceKey, MaskAllocator allocator) throws
HopException {
+ String patternName, String sourceKey, KeySupplier legacyKey,
MaskAllocator allocator)
+ throws HopException {
Map<String, String> patternCache =
cache.computeIfAbsent(patternName, k -> new ConcurrentHashMap<>());
String cached = patternCache.get(sourceKey);
if (cached != null) {
return cached;
}
ensureOpen();
- try {
- String existing = lookup(patternName, sourceKey);
- if (existing != null) {
+ for (int attempt = 1; ; attempt++) {
+ try {
+ String masked = lookup(patternName, sourceKey);
+ String oldKey = masked == null && legacyKey != null ? legacyKey.get()
: null;
Review Comment:
**[suggestion]** The legacy key is read only when the new key is missing.
After ignore-case, trim, or hashing folds two stored spellings onto one key,
the first spelling is rekeyed and the other plain-text row is left in place.
The next time that second value is masked, `lookup` hits the new key and this
branch never runs, so `source_key` still holds the original. The manual says a
plain-text row moves the next time its value is masked.
**Suggestion:** When the legacy key differs from `sourceKey`, delete that
old row after the new key is found or written. If both rows exist, keep the
replacement stored under the new key. Cover two stored casings in
`DatabaseMaskingStoreTest`.
##########
plugins/transforms/maskfields/src/main/java/org/apache/hop/pipeline/transforms/maskfields/MaskingKey.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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.
+ */
+
+package org.apache.hop.pipeline.transforms.maskfields;
+
+import java.math.BigDecimal;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.util.HexFormat;
+import java.util.Locale;
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hop.core.encryption.Encr;
+import org.apache.hop.core.exception.HopException;
+import org.apache.hop.core.exception.HopValueException;
+import org.apache.hop.core.row.IValueMeta;
+import org.apache.hop.core.util.StringUtil;
+import org.apache.hop.core.variables.IVariables;
+import org.apache.hop.i18n.BaseMessages;
+
+/**
+ * Builds the key a remembering pattern finds a source value by. The key
ignores the field's
+ * conversion mask, can ignore spaces around the value and its case, and is an
HMAC of the value
+ * when the pattern has a hash secret.
+ */
+public final class MaskingKey {
+
+ private static final Class<?> PKG = MaskingKey.class;
+
+ static final String HMAC_PREFIX = "hmac-sha256:";
+ private static final String ALGORITHM = "HmacSHA256";
+
+ private final boolean trim;
+ private final boolean ignoreCase;
+ private final Mac mac;
+
+ public MaskingKey(boolean trim, boolean ignoreCase, String secret) throws
HopException {
+ this.trim = trim;
+ this.ignoreCase = ignoreCase;
+ if (StringUtils.isEmpty(secret)) {
+ this.mac = null;
+ } else {
+ try {
+ Mac hmac = Mac.getInstance(ALGORITHM);
+ hmac.init(new SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8),
ALGORITHM));
+ this.mac = hmac;
+ } catch (GeneralSecurityException e) {
+ throw new HopException("Unable to initialize " + ALGORITHM, e);
+ }
+ }
+ }
+
+ /**
+ * The key for a pattern. Only a database pattern hashes its keys. A hash
secret that resolves to
+ * nothing, or still holds a variable, is an error: hashing with the literal
text or falling back
+ * to plain text would both go unnoticed.
+ */
+ public static MaskingKey forPattern(MaskingPattern pattern, IVariables
variables)
+ throws HopException {
+ String secret = null;
+ if (pattern.getStorage() == MaskingStorage.DATABASE
+ && StringUtils.isNotEmpty(pattern.getHashSecret())) {
+ String resolved = variables.resolve(pattern.getHashSecret());
+ if (StringUtils.isEmpty(resolved) ||
StringUtil.containsVariableToken(resolved)) {
+ throw new HopException(
+ BaseMessages.getString(
+ PKG, "MaskFields.Error.HashSecretUnresolved",
pattern.getName()));
+ }
+ secret = Encr.decryptPasswordOptionallyEncrypted(resolved);
Review Comment:
**[bug]** An empty or still-unresolved hash secret aborts startup, but a
secret that does not decrypt does not.
`Encr.decryptPasswordOptionallyEncrypted` returns an empty string when an
`Encrypted ` payload is not a valid ciphertext, and `MaskingKey` treats an
empty secret as "do not hash". The pattern then writes `source_key` in plain
text with no error, which the comment on this method says must not happen
unnoticed. The metadata loader leaves a variable reference such as
`${MASKING_SECRET}` for this method to decrypt, so a broken `Encrypted ...`
value in the variable takes this path.
**Suggestion:** When the pattern has a hash secret, throw
`MaskFields.Error.HashSecretUnresolved` if the decrypted secret is empty. Do
not build a key with a null MAC in that case.
--
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]
