dsmiley commented on code in PR #60:
URL: https://github.com/apache/solr-sandbox/pull/60#discussion_r1220121924
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -238,36 +245,39 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
private void commitEmptyIndexForEncryption(@Nullable String keyId,
SegmentInfos segmentInfos,
- SolrQueryRequest req) throws
IOException {
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws
IOException {
// Commit no change, with the new active key id in the commit user data.
log.debug("commit on empty index for keyId={}", keyId);
CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, false);
commitCmd.commitData = new HashMap<>(segmentInfos.getUserData());
commitCmd.commitData.remove(COMMIT_ENCRYPTION_PENDING);
- setNewActiveKeyIdInCommit(keyId, commitCmd, req);
+ setNewActiveKeyIdInCommit(keyId, commitCmd, req, rsp);
assert !commitCmd.commitData.isEmpty();
req.getCore().getUpdateHandler().commit(commitCmd);
}
- private void setNewActiveKeyIdInCommit(String keyId, CommitUpdateCommand
commitCmd, SolrQueryRequest req)
- throws IOException {
+ private void setNewActiveKeyIdInCommit(String keyId,
Review Comment:
_(I'm just rambling; no action asked)_ note that an UpdateCommand contains
the SolrQueryRequest, albeit not the SolrQueryResponse. So it's kind of
redundant to supply req. I wish the SolrQueryResponse instance, in Solr, was
retrievable from the SolrQueryRequest as it's super common to want both, and
they are effectively paired.
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -164,8 +196,39 @@ public static void
clearOldInactiveKeyIdsFromCommit(Map<String, String> commitUs
inactiveKeyRefs.sort(Comparator.naturalOrder());
for (Integer keyRef : inactiveKeyRefs.subList(0, inactiveKeyRefs.size()
- INACTIVE_KEY_IDS_TO_KEEP)) {
commitUserData.remove(COMMIT_KEY_ID + keyRef);
- commitUserData.remove(COMMIT_KEY_COOKIE + keyRef);
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.remove(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ for (int i = 0; i < cookieSize; i++) {
+ String entryPrefix = commitKeyCookie + '.' + i + '.';
+ commitUserData.remove(entryPrefix + 'k');
+ commitUserData.remove(entryPrefix + 'v');
+ }
+ }
}
}
}
+
+ /**
+ * Key cookie key-value pairs optionally associated to a key id in the
commit user data.
+ */
+ public static class KeyCookies {
+
+ private static final KeyCookies EMPTY = new KeyCookies(Map.of());
+
+ private final Map<String, Map<String, String>> cookies;
Review Comment:
cookiesByKey would be clearer
##########
encryption/src/main/java/org/apache/solr/encryption/KeySupplier.java:
##########
@@ -17,59 +17,64 @@
package org.apache.solr.encryption;
import org.apache.lucene.index.IndexFileNames;
+import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
+import javax.annotation.Nullable;
import java.io.IOException;
+import java.util.Map;
import java.util.function.Function;
/**
- * Manages encryption keys and defines which index files to encrypt.
- * Supplies the encryption key secrets corresponding to provided key ids.
+ * Provides encryption key secrets corresponding to provided key ids and
defines which index files to encrypt.
*/
-public interface KeyManager {
+public interface KeySupplier {
/**
- * Indicates whether the provided file is encryptable based on its name.
+ * Indicates whether the provided file should be encrypted based on its name.
* <p/>
* Segments files ({@link IndexFileNames#SEGMENTS} or {@link
IndexFileNames#PENDING_SEGMENTS}) are never
* passed as parameter because they are filtered before calling this method
(they must not be encrypted).
*/
- boolean isEncryptable(String fileName);
+ boolean shouldEncrypt(String fileName);
/**
- * Gets the cookie corresponding to a given key.
- * The cookie is an additional binary data to provide to get the key secret.
+ * Gets the optional cookie corresponding to a given key.
+ * The cookie is a set of key-value pairs to provide to {@link
#getKeySecret}.
*
+ * @param params optional parameters in addition to the key id; or null if
none.
+ * @return the key-value pairs; or null if none.
* @throws java.util.NoSuchElementException if the key is unknown.
*/
- byte[] getKeyCookie(String keyId) throws IOException;
+ @Nullable
+ Map<String, String> getKeyCookie(String keyId, Map<String, String> params)
throws IOException;
/**
* Gets the encryption key secret corresponding to the provided key id.
+ * Typically, this {@link KeySupplier} holds a cache of key secrets, and may
load the key secret if it is
+ * not in the cache or after expiration. In this case, this method may need
to get additional data from
+ * the key cookie to load the key secret.
*
* @param keyId Key id which identifies uniquely the encryption key.
- * @param keyRef Key internal reference number to provide to the
cookie supplier to retrieve the
- * corresponding cookie, if any.
- * @param cookieSupplier Takes the key reference number as input and
supplies an additional binary data
- * cookie required to get the key secret. This
supplier may not be called if the
- * key secret is in the transient memory cache. It may
return null if there are no
- * cookies.
+ * @param cookieSupplier Takes the key id as input and supplies the optional
key cookie key-value pairs
+ * that may be needed to retrieve the key secret. It
may return null if there are
+ * no cookies for the key.
* @return The key secret bytes. It must be either 16, 24 or 32 bytes long.
The caller is not permitted
* to modify its content. Returns null if the key is known but has no secret
bytes, in this case the data
* is not encrypted.
* @throws java.util.NoSuchElementException if the key is unknown.
*/
- byte[] getKeySecret(String keyId, String keyRef, Function<String, byte[]>
cookieSupplier) throws IOException;
+ byte[] getKeySecret(String keyId, Function<String, Map<String, String>>
cookieSupplier) throws IOException;
/**
- * Supplies the {@link KeyManager}.
+ * Creates {@link KeySupplier}.
*/
- interface Supplier {
+ interface Factory {
- /** This supplier may be configured with parameters defined in
solrconfig.xml. */
- void init(NamedList<?> args);
Review Comment:
solrconfig things are NamedList; why use Params? Should have implemented
NamedListInitializedPlugin
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionRequestHandler.java:
##########
@@ -238,36 +245,39 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
private void commitEmptyIndexForEncryption(@Nullable String keyId,
SegmentInfos segmentInfos,
- SolrQueryRequest req) throws
IOException {
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws
IOException {
// Commit no change, with the new active key id in the commit user data.
log.debug("commit on empty index for keyId={}", keyId);
CommitUpdateCommand commitCmd = new CommitUpdateCommand(req, false);
commitCmd.commitData = new HashMap<>(segmentInfos.getUserData());
commitCmd.commitData.remove(COMMIT_ENCRYPTION_PENDING);
- setNewActiveKeyIdInCommit(keyId, commitCmd, req);
+ setNewActiveKeyIdInCommit(keyId, commitCmd, req, rsp);
assert !commitCmd.commitData.isEmpty();
req.getCore().getUpdateHandler().commit(commitCmd);
}
- private void setNewActiveKeyIdInCommit(String keyId, CommitUpdateCommand
commitCmd, SolrQueryRequest req)
- throws IOException {
+ private void setNewActiveKeyIdInCommit(String keyId,
+ CommitUpdateCommand commitCmd,
+ SolrQueryRequest req,
+ SolrQueryResponse rsp) throws
IOException {
if (keyId == null) {
removeActiveKeyRefFromCommit(commitCmd.commitData);
ensureNonEmptyCommitDataForEmptyCommit(commitCmd.commitData);
} else {
- byte[] keyCookie = getKeyManager(req).getKeyCookie(keyId);
+ KeySupplier keySupplier = ((EncryptionDirectoryFactory)
req.getCore().getDirectoryFactory()).getKeySupplier();
+ Map<String, String> keyCookie = keySupplier.getKeyCookie(keyId,
buildGetCookieParams(req, rsp));
EncryptionUtil.setNewActiveKeyIdInCommit(keyId, keyCookie,
commitCmd.commitData);
}
}
- private KeyManager getKeyManager(SolrQueryRequest req) {
- try {
- return ((EncryptionDirectoryFactory)
req.getCore().getDirectoryFactory()).getKeyManager();
- } catch (ClassCastException e) {
- throw new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE,
- "DirectoryFactory class must be set to " +
EncryptionDirectoryFactory.class.getName() + " to use " +
getClass().getSimpleName(),
- e);
- }
+ /**
+ * Can be extended to build cookie params based on the request.
Review Comment:
Normally methods start by saying what it does, and not wether or not it can
be extended (the "protected" keyword basically speaks for itself). But okay.
Is the map returned to be considered immutable?
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String>
commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided
commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String>
commitUserData) {
+ Map<String, Map<String, String>> keyCookies = null;
+ for (Map.Entry<String, String> dataEntry : commitUserData.entrySet()) {
+ if (dataEntry.getKey().startsWith(COMMIT_KEY_ID)) {
+ String keyId = dataEntry.getValue();
+ String keyRef = dataEntry.getKey().substring(COMMIT_KEY_ID.length());
+ String commitKeyCookie = COMMIT_KEY_COOKIE + keyRef;
+ String cookieSizeAsString = commitUserData.get(commitKeyCookie);
+ if (cookieSizeAsString != null) {
+ int cookieSize = Integer.parseInt(cookieSizeAsString);
+ Map<String, String> cookie = new HashMap<>((int) (cookieSize /
0.75f) + 1);
Review Comment:
Honestly, I never know if that argument is valid or not and I suspect other
readers won't either. :-) I suggest not over-engineering this right now but
maybe add a convenience method for hashMap sizing -- not sure if we got to that
in Solr yet. I remember a discussion with @risdenk about it with you.
##########
encryption/src/main/java/org/apache/solr/encryption/EncryptionUtil.java:
##########
@@ -142,6 +144,36 @@ public static boolean hasKeyIdInCommit(Map<String, String>
commitUserData) {
return false;
}
+ /**
+ * Gets the cookies (key-value pairs) for all the key ids, from the provided
commit user data.
+ *
+ * @return the cookies for all key ids.
+ */
+ public static KeyCookies getKeyCookiesFromCommit(Map<String, String>
commitUserData) {
Review Comment:
This is awkward. Should we use JSON to embed data? Just a thought; needn't
change.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
