You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/06/14 14:30:10 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2777: Drop Serializable from Authorizations
milleruntime opened a new pull request, #2777:
URL: https://github.com/apache/accumulo/pull/2777
It looks like all the serialization done for `Authorizations` is done with custom methods that we have created over the years. I am not sure why we need the interface on the class anymore.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1156341257
> Can't you just make ByteSequence serializable? ArrayList and HashSet already are.
It looks like we may not have to. By changing the private types from interfaces to classes, the warnings went away:
```diff
- private Set<ByteSequence> auths = new HashSet<>();
- private List<byte[]> authsList = new ArrayList<>(); // sorted order
+ private final HashSet<ByteSequence> auths = new HashSet<>();
+ private final ArrayList<byte[]> authsList = new ArrayList<>(); // sorted order
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155924807
Can't you just make ByteSequence serializable? ArrayList and HashSet already are.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155498652
> > The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?
>
> Is there a security flaw?
I don't know.
> I believe the solution for #2776 in regards to this class is to mark these fields as `transient`
That may be a better fix. But I think this would still break the serialization if a user extended Authorizations. I am going to write a test to experiment.
My point being, if we are going to break the serialization to fix the warning, lets just get rid of it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1156362659
If a user is doing the following:
```
Serializable auths = new Authorations();
```
but never serializes or deserializes the object, then according to the [JLS](https://docs.oracle.com/javase/specs/jls/se6/html/binaryComp.html#13.4.4) they will get a `VerifyError`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155479506
> I believe that this is part of the [public API](https://accumulo.apache.org/api/).
Yeah, its definitely in the public API. I am not sure what the use case would be for using the Java serialization of the Auths though.
> I'm not sure if this violates our policy, but I think it might.
The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2777: Make auths classes in Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2777:
URL: https://github.com/apache/accumulo/pull/2777
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155511285
If you remove the `Serializable` interface from the class, then I believe that breaks binary compatibility, which violates our public API policy. If the serailization/deserialization of Authorizations is done by other means, then adding `transient` may not break anything. If this is just a `warning` put out by the Java 18 JVM, then we might not need to do anything.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155406468
I believe that this is part of the [public API](https://accumulo.apache.org/api/). I'm not sure if this violates our policy, but I think it might.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155933708
Its kinda neat that Java 18 found this. @milleruntime would you happen to have the error/warn message? I am curious what it said.
Trying to understand the implications of removing the interface on binary compat I was looking at :
https://wiki.eclipse.org/Evolving_Java-based_APIs_2
Reading that doc is seems like removing it may only break binary compat in the case where code is casting to Serializable. Does that interpretation seem correct? Since serialization did not work, maybe no one was doing that and its ok to remove if code that was not casting to Serializable is ok.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1156336596
> Its kinda neat that Java 18 found this. @milleruntime would you happen to have the error/warn message? I am curious what it said.
I currently only have Java 18 in my IDE and this is all it is saying:
<pre>
workspace/accumulo/core/src/main/java/org/apache/accumulo/core/security/Authorizations.java:47:29
java: non-transient instance field of a serializable class declared with a non-serializable type
</pre>
> Reading that doc it seems like removing it may only break binary compat in the case where code is casting to Serializable. Does that interpretation seem correct?
I will take a look at that link, thanks.
> Since serialization did not work, maybe no one was doing that and its ok to remove if code that was not casting to Serializable is ok.
So the serialization of `Authorizations` _does_ work as it is currently written. See https://github.com/apache/accumulo/pull/2777#issuecomment-1155517364. I am going to create a test for all the Serialization warnings so we can figure out a way forward for https://github.com/apache/accumulo/issues/2776.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155486659
> The question would be, if keeping the class serializable exposes a possible security flaw... is it worth breaking any serialization that a user may be using?
Is there a security flaw?
I believe the solution for #2776 in regards to this class is to mark these fields as `transient`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155463264
This change is causing a failure in `FateIT.testTransactionStatus()`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155549721
> in the case where you marked the fields transient, then you just need to protect against NPE in equals() ?
>
> EDIT: I think your serialization / deserialization worked with `transient`, the `assertEquals` caused it to fail
The `equals()` is a problem but its more than that. The `toString()` also needs rewritten. Probably the hashCode() method as well.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155517364
I wrote a simple test that writes and reads the Authorizations object and it passes on main. I think changing the serialization breaks the API either way:
<pre>
fields marked transient:
Exception in toString(): java.lang.NullPointerException: Cannot invoke "java.util.Set.iterator()" because "this.auths" is null
Dropped serializable:
java.io.NotSerializableException: org.apache.accumulo.core.security.Authorizations
</pre>
Here is the test I added to AuthorizationsTest:
<pre>
@Test
public void testSerializable() throws Exception {
ByteArrayOutputStream bytesOut = new ByteArrayOutputStream();
ObjectOutputStream outputStream = new ObjectOutputStream(bytesOut);
Authorizations auths = new Authorizations("foo");
outputStream.writeObject(auths);
outputStream.flush();
ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesOut.toByteArray());
ObjectInputStream inputStream = new ObjectInputStream(bytesIn);
Authorizations readObject = (Authorizations) inputStream.readObject();
assertEquals(auths, readObject);
}
</pre>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2777: Drop Serializable from Authorizations
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2777:
URL: https://github.com/apache/accumulo/pull/2777#issuecomment-1155520536
in the case where you marked the fields transient, then you just need to protect against NPE in equals() ?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org