There are a few internal state objects in MslControl that are created and keyed off external data (e.g. MslContext, MasterToken). If the calling application destroys those objects, the internal state objects need to be destroyed too.

The current implementation of SimpleMslStore.addServiceTokens() will throw an exception the first time it encounters a service token that is master token- or user ID token-bound and it does not have a matching master token or user ID token.

It may instead be better to delay throwing the exception until all service tokens have been processed. Or to instead not add any service tokens until we are sure all of them can be added.

Looks like this is because of this commit: f9cb4af

It was a jar in this version:
https://bintray.com/netflixoss/maven/msl/1.1199.0#files/com/netflix/msl/msl-cli/1.1199.0

But a war here:
https://bintray.com/netflixoss/maven/msl/1.1204.0#files/com/netflix/msl/msl-cli/1.1204.0

Best practice is to use new key request data after each successful key exchange occurs. Currently the only good indicator of this within the MSL stack is to have the MslStore implementation trigger creation of new key request data upon receipt of a new master token. But that is sort of a strange approach since the MslStore is not in charge of and not directly related to key request data or the key exchange operation.

Come up with some new way of better signaling that new key request data should be used for any future messages.

The MessageContext must remain the interface that answers whether or not key request data is included in a message to allow applications to explicitly exclude key request data from a message as desired, and for maximum flexibility also retain ownership for providing the actual key request data. Therefore the new signal must affect any existing MessageContext instances in addition to ones created after the signal is triggered.

RsaCryptoContext.js currently defines wrap/unwrap with key format 'jwk' hard coded. This should be a parameter somewhere.

One of the MslControl.request() functions accepts a URL parameter. However Java has defined this class as final preventing implementers from overriding it, and thus the only way to provide a different implementation is by registering URL handlers which is inconvenient and relatively complicated.

This parameter could be replaced by a different URL interface similar to what was done in the JavaScript implementation.

The TokenFactory.isNewestMasterToken() function was originally added to address the need to move a client onto the newest master token sequence number to support the creation of non-replayable messages when a client is currently on an older master token sequence number, when the non-replayable feature was based off the master token sequence number.

Since moving to the non-replayable ID, that functionality is no longer necessary. Instead, the only purpose this function serves today is to immediately force a master token renewal if we believe the client should be using a newer one. This results in an immediate check for a cloned entity.

Removal of this function would remove the immediate check for a cloned entity, pushing that check out until the master token renewal window is entered or the master token is expired. This is arguably bad. Alternatively provide more documentation around the purpose of this function (function Doxygen and/or Wiki).

MslControl converts exceptions (via MessageBuilder.createErrorResponse) into a ErrorHeader and passes that to MessageDebugContext. Those exceptions have very valuable information for logging and diagnostics purposes. I would like the exception to be passed class that outside implementations control, like how MessageDebugContext is provided to MSL and can log headers. It could even be a new call on MessageDebugContext (which could be added a Java 8 default method on the interface), e.g. MessageDebugContext.exceptionFound(Exception). Or as a second parameter to sendHeader, but that's awkward if there is header is a MessageHeader. Maybe MessageDebugContext, should be refactored to have a sentMessageHeader(MessageHeader) and a sentErrorHeader(ErrorHeader, Throwable). This would make it easier for implementors too, since we wouldn't have to do an instance of on the Header and do the delegation ourself.

Allow MSL configurations to specify a maximum size for data processed by a crypto operation, to ensure compatibility with all participating entities. Perhaps this belongs in MessageCapabilities. Application data can be split across MSL payload chunks to satisfy this limit, but errors would need to be thrown for data in tokens and headers.

The default size should be set to 100KiB.

The underlying MslException accepts a Throwable as a cause, but MslUserIdTokenException doesn't.

PrivateKey currently only supports import of Base64-encoded or raw PKCS#8 private keys. PublicKey currently only supports import of Base64-encoded or raw SPKI public keys. They should also support other key formats, like JWK.

Once this is done, the logic in EccCryptoContextSuite.js can be fixed up to use those classes instead of calling Web Crypto functions directly, which is less manageable.

Some of the MslEncoderUtils functions, like equality, merge, and hash could be moved into MslObject and MslArray. This also has the side benefit of allowing some otherwise public functions like getMap() and getCollection() to either be deleted or hidden.

With -v true, it prints something like:

{"headerdata":"X","signature":"Y","entityauthdata":{"scheme":"PSK","authdata":{"identity":"I"}}}{"headerdata":"X","signature":"Y","entityauthdata":{"scheme":"PSK","authdata":{"identity":"I"}}}{"payload":"P","signature":"S"}{"payload":"P","signature":"S"}
Error: Server returned HTTP response code: 400 for URL: http://xyz/uri

But the full HTTP request actually looks like this:

POST /scriptManager?hash=None&hasChronosSync=true&action=RUN&v=1.1&debug=true HTTP/1.1
User-Agent: Java/1.8.0_60
Host: 127.0.0.1:7001
Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2
Connection: keep-alive
Content-type: application/x-www-form-urlencoded
Content-Length: 1458

{"headerdata":"X","signature":"Y","entityauthdata":{"scheme":"PSK","authdata":{"identity":"I"}}}{"payload":"P","signature":"S"}

Missing:

• HTTP verb and version
• HTTP headers for the request
• HTTP headers for the response
• HTTP payload of the response

By having the documentation in the repository directly, it can be updated/committed/submitted at the same the feature/change is made. Github allows editing files in the repository directly, which means we keep the ability to update the docs via the website, just like in the wiki.

It also opens the possibility to somehow syncing the docs with the code some day.

Add explicit support for application-controlled one-directional perfect forward secrecy when it matters not if it the request data is protected using PFS but the response data needs to be protected using PFS. This may be useful when the request data is relatively useless but the response data does require protection.

One-directional support for PFS allows you to avoid a the extra network round trip needed to establish session keys negotiated using a PFS key exchange scheme.

Similar to how the authentication and keyx schemes can be extended by the MSL configuration layer, doing the same for the compression and crypto enums currently held in MslConstants will probably also be useful.

MslUtils would need to allow compression algorithms to be installed as well (maybe similar to Base64). Perhaps a compression class should be created explicitly to support compression implementations instead of continuing to mix it into MslUtils.

Google Test supports static and global environment setup/teardown which will allow expensive initialization like the MockMslContext and other constants to be initialized one during unit tests instead of on every single test like it is currently implemented. This should help speed up execution of and reduce resource consumption of the tests.

See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md for more details.

ECDSA is not supported on all browsers.
We should create a filter to exclude these tests from browsers that lack ECDSA support.

The current documentation for MslControl indicates the Future may throw a MslException or IOException. It would be clearer to indicate that the Future's thrown ExecutionException may have a MslException or IOException for its cause.

C++ defines and uses the Date type differently than Java + JavaScript, such that the instance can never be null but must instead be queried for validity. This unfortunately does not match with the documentation or implementation of the MslControl .setRemoteTime() and .getRemoteTime() functions.

For safety, recast the Date type to be a shared_ptr and thus nullable and capable of matching the design and logic of the Java and JavaScript code. Ensure the documentation and implementation is fixed to match.

Currently, this is resulting in a bug when MslControl asks MslContext for the remote time and then passes it onto the token renewable/expired query functions.

The same approach of using concrete instances that must be queried for validity applies to some other types as well. Similar problems may also be lurking for those types.

The toString() methods on master token, user ID token, and service token classes may leak sensitive data by including the otherwise encrypted data if the local entity was able to decrypt it. This data should not be included to avoid accidents where a token's string representation is included in a log message.

Base64Secure could throw another exception when a character is not a base64 character, which would indicate that the input data is corrupt.

Caused by: java.lang.ArrayIndexOutOfBoundsException: 65533 at com.netflix.msl.util.Base64Secure.decode(Base64Secure.java:138) at com.netflix.msl.util.Base64.decode(Base64.java:103) at com.netflix.msl.tokens.MasterToken.<init>(MasterToken.java:307) at com.netflix.msl.msg.Header.parseHeader(Header.java:123) at com.netflix.msl.msg.MessageInputStream.<init>(MessageInputStream.java:221) at com.netflix.msl.msg.NetflixMessageInputStream.<init>(NetflixMessageInputStream.java:82) at com.netflix.msl.msg.NetflixMessageStreamFactory.createInputStream(NetflixMessageStreamFactory.java:94) at com.netflix.msl.msg.MslControl.receive(MslControl.java:1645) at com.netflix.msl.msg.MslControl.access$400(MslControl.java:188) at com.netflix.msl.msg.MslControl$ReceiveService.call(MslControl.java:2214) ... 62 more

There is a desire to provide some sort of non-global / constant data to the execution of what are otherwise singletons off MslContext: authentication factories, keyx factories, token factory, MSL store, etc.

Passing some sort of random Object throughout the entire MSL code base into every possible configuration implementation function seems both incredibly messy, wrong, and error prone in the same sense as globals. Additionally there's no guarantee the Object data would be thread-safe, neither of which is currently a requirement on the MslContext, MessageContext, and other interfaces. Sticking something on MessageContext would fundamentally change MessageContext which can currently be treated as a stateless data resource.

This request requires further thought before deciding it is a good idea and if so then how to best approach it.

The current MSL specification and code specifies the use of JSON for encoding sections of MSL messages, and defines a MSL message as concatenations of JSON objects.

JSON is particularly heavyweight for communication due to its use of text for non-text values, and the need to Base64-encode any binary data which immediately increases data size by 33%.

Change the MSL code so the actual encoding portions can be abstracted out, and also add support for a binary encoding that is still as flexible, extensible, and backwards/forwards-compatible as JSON.

Some of the injectable classes are currently defined as abstract or inheritable base classes. This is mostly to provide a fully functional implementation that can be extended or have methods overridden for customization purposes. However from a design standpoint it may be better to define them as pure interfaces with the current base class becoming a concrete example.

The regex use doesn't take into account newlines, as is common in certificates, e.g.

TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQsIGNvbnNlY3RldHVyIGFkaXBpc2NpbmcgZW
xpdCwgc2VkIGRvIGVpdXNtb2QgdGVtcG9yIGluY2lkaWR1bnQgdXQgbGFib3JlIGV0IGRvb
G9yZSBtYWduYSBhbGlxdWEuIFV0IGVuaW0gYWQgbWluaW0gdmVuaWFtLCBxdWlzIG5
vc3RydWQgZXhlcmNpdGF0aW9uIHVsbGFtY28gbGFib3JpcyBuaXNpIHV0IGFsaXF1aXAg
ZXggZWEgY29tbW9kbyBjb25zZXF1YXQuIER1aXMgYXV0ZSBpcnVyZSBkb2xvciBpbiByZ
XByZWhlbmRlcml0IGluIHZvbHVwdGF0ZSB2ZWxpdCBlc3NlIGNpbGx1bSBkb2xvcmUgZX
UgZnVnaWF0IG51bGxhIHBhcmlhdHVyLiBFeGNlcHRldXIgc2ludCBvY2NhZWNhdCBjdXBp
ZGF0YXQgbm9uIHByb2lkZW50LCBzdW50IGluIGN1bHBhIHF1aSBvZmZpY2lhIGRlc2Vyd
W50IG1vbGxpdCBhbmltIGlkIGVzdCBsYWJvcnVtLg==

In the case of Jaxb, the underlying implementation can support newlines just fine. Instead of introducing an uncompiled Regex to ever call, just let the underlying implementation take care of validating. I think the isValidBase64 call should be removed from the decode method.

The whole library is using insecure time-variable comparison to compare secrets. This makes it vulnerable to timing attacks.

Example

return Arrays.equals(hmac, envelope.getSignature());

Include a safe comparision function in your library and use it everywhere. An example of such function

public static boolean safeEquals(byte[] a, byte[] b) {
   if (a.length != b.length) {
      return false;
   }

   int result = 0;
   for (int i = 0; i < a.length; i++) {
      result |= a[i] ^ b[i]
   }
   return result == 0;
}

^{Credit to Coda Hale for the function}

MslControl currently accepts InputStream, OutputStream, or the MSL Url types for communication with the remote entity. Maybe this should be abstracted through some sort of remote entity connection interface, because it may be necessary to "close" and "open" connections for each MSL message or request/response pair.

This may be problematic for 3-message transactions, and due to expectation that a MslChannel is established and can be used for full-duplex communication. It may be that introducing this abstraction would be a bad idea because it breaks the expectation that an underlying protocol should be full-duplex and that overlay protocols like MSL should be agnostic. HTTP is the odd one out in that respect.

When handling an exception, some functions may throw another exception. These second exceptions should be discarded in favor of the original exception. There appears to be a few locations where this behavior is not honored (e.g. RequestService.call() when catching IOException or RuntimeException and closing the streams).

Add an advanced mode to the example JavaScript MSL client that lets you manipulate the message properties and application payloads directly.

This is a questionable feature request: allow individual payloads, or all payloads, to be sent without encryption even though the MSL stack is capable of providing encryption.

If I have a MslUserAuthException and then I call .setUser(userIdToken), it becomes a MslException, which then requires a broader method signature than is needed. Is it possible to add a generic type to MslException so that the child exception type can be kept?

Since USERDATA_REAUTH may be returned in some cases when a user ID token is being used (when the responder wishes to force a user to provide new user authentication data) the MslControl.cleanupContext() function should remove the user ID token associated with the user if one exists.

Double-check the Wiki documentation to make sure this change in behavior is documented.

Define one unit test timeout for MockMslContext initialization, and another for the typical unit test timeout value.

There is a call chain in MslControl.java that may result in attempting to release a master token read lock when it has already been released. RequestService.execute() and RespondService.peerToPeerExecute() will call cleanupContext() upon receipt of error messages. This will in turn call deleteMasterToken() if the the error is ENTITY_REAUTH. However deleteMasterToken() assumes that if the master token lock exists, then the caller currently holds the read lock; this happens to be true if being called from cleanupCryptoContext().

Possible solutions:

• Move the call to cleanupContext() inside of sendReceive(), before the master token lock is released. This would be consistent with the calls to cleanupCryptoContext(). There do not appear to be any potential side effects that would discourage using this solution.
• Change cleanupContext() so when it tries to delete a master token it knows the read lock has already been released. This adds a little more complexity and duplicates part of an existing function so this solution doesn't look as good.

EccCryptoContext.js documentation states it accepts PrivateKey and PublicKey types, consistent with other ICryptoContext implementations. However the code is written to operate on raw key handles, and EccCryptoContextSuite.js passes in raw key handles.

For an example of the correct behavior, see the RsaCryptoContext.js ctor which pulls out the raw keys from the input parameters.

At very large scale, the 2^53 number space for MSL message IDs may be too small to sufficiently prevent replayed responses. Consider supporting BigInteger values of arbitrary length (although the implementation should specify a maximum size) or of specified but much larger bit size.

To match issue #51, remove the master token if it exists inside MslControl.cleanupContext() if ENTITYDATA_REAUTH is triggered.

Likewise, update the Wiki documentation to match this changed behavior.

Define a logging interface and add log callbacks to the core code to provide insight into MSL logic.

Some requirements:

• Function entry/exit.
• Decision making.
• Error severities.
• Logging areas.

SLF4J (Simple Logging Facade for Java) was suggested but requires inclusion of a specific library/JAR (slf4j-api.jar) which can cause problems. It may serve as a useful design example.

There is a performance concern with always calling the log functions, and building their arguments, even if the log message will be discarded. An outer if statement is one way to address this.

The master token and session crypto context is currently hard-coded to assume AES/CBC/PKCS5Padding and HmacSHA256 algorithms. It should be possible to override these default choices in case different algorithms should be used.

MslControl.acquireRenewalLock() waits to receive a master token from the currently renewing request. However if the call to ctxRenewingQueue.poll() times out then the return value is null and the the operation must abort and time out. Currently the code continues assuming the master token was returned, and this will throw a null pointer exception.

https://github.com/Netflix/msl/blob/master/core/src/main/java/com/netflix/msl/tokens/UserIdToken.java#L79

The userdata documentation for the UserIdToken says "user" is a mandatory field, but it's not listed as an actual field. I believe the mandatory field is identity.

When using Gradle with the Gretty plugin to deploy the example simple server, the application is deployed under /msl-example instead of under /msl-example-server as defined by the gradle.build variable war.baseName.

The example files are defined to point at /msl-example-server so this creates a problem when people run with the default settings.

If Key Exchange or Entity Auth learn something about an entity, they should be able to contribute entity bound service tokens.

User Auth Schemes should also be able to contribute user-bound tokens.

KeyX/EntityAuth being able to contribute User bound tokens, though not the other way, ie. User Auth Scheme shouldn't be able to provide Entity bound service tokens. This would also be for deficient implementations that only store user-bound tokens.

Many of the current entity authentication schemes require an entity identification value to be passed in the clear, as this is the only way for a receiver to identify the cryptographic keys or algorithms necessary for performing authentication. However this is a privacy concern.

Add support master token-based wrapping of entity authentication schemes. This would allow an initial key exchange and issuance of ephemeral session keys to occur without any strong entity identity (i.e. a randomly generated identity) to be used to protect the actual entity authentication data.

The Master Token Protected entity authentication scheme was created to allow an entity to establish temporary session keys to secure the delivery of the real entity authentication data.

However, the recipient field will leak the remote entity's identity when creating a response and encryption is not available.

The recipient field was not originally specified. They were proposed as a way to prevent messages generated by one trusted services server from being replayed against another trusted services server, under the thinking that communication should only occur between the client and servers in a trusted services network, and never between servers themselves. At the time I argued against this change because I didn't think that would matter, as from the MSL perspective nothing bad could happen (server messages will not contain key request data, and never use the explicit non-replayable feature and instead relying on the message ID) and the application running on top of MSL would still need to process application data properly regardless—there is nothing stopping a rogue client from sending a message with application data matching a server's application data.

The recipient field could be removed, or only included when the header will be encrypted.

mslcli.common.IllegalCmdArgumentException: User Authentication Scheme is not set is thrown after entering MslControl$RequestService.call(), which is completely unexpected.

MslControl.cancelled() considers InterruptedIOException as an application-initiated, or at least application-aware, exception. However SocketTImeoutException is a subclass of InterruptedIOException and will be thrown due to socket timeout instead of due to cancellation or thread interruption. Make sure it is properly delivered as a timeout to the caller instead of being hidden as a cancellation.

Check if there are other exceptions that fall in under this category. Primarily IOExceptions that will be thrown by the InputStream and OutputStreams.

MslControl currently uses MslContext as a data key for a few things, as well as uses MslContext to hold the remote entity clock. This is a potential problem since it's not clear to the MSL configuration that MslContext needs to serve a role as a data key. MslStore might be slightly better, since it is obvious that contains mutable state data.

The call to request.isHandshake() inside ReceiveService.call() should catch InterruptedException and MslException before catching Throwable, in order to provide the proper error response data. Make sure to check MslControl in JavaScript and C++ as well.