Pax User Admin

Credentials encryption mechanism breaks contract

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: Service
  • Labels:
    None

Description

The encryption mechanism breaks 3rd party code because it breaks the UserAdmin contract by implicitly encrypting values and, in case of String, even changing type into byte[]. The latter also happens when no encryption is used. For example, the second line below causes a ClassCastException because a byte[] is returned instead of the expected String.

> ((User) r).getCredentials().put("password", "secret");
> String pwd = (String) ((User) r).getCredentials().get("password");

From the spec I get that access to plain text credentials should be protected through a UserAdminPermission. Probably should leave encryption of persisted data to the storage provider or make it part of the spi.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Matthias Küspert added a comment -

Yes: that the value type is changed even when not using encryption is definitely a bug.

However if values should be encrypted I see no other way than to change the type - otherwise a getCredentials() call would have to return a String with non-ascii characters. Moving the encryption logic to the SPI would IMHO not change this behaviour.

The encryption feature is just something I saw in the Felix implementation of the UserAdmin service (https://github.com/apache/felix/tree/trunk/useradmin). It seemed a nice-to-have feature at that time, but I'm not so sure anymore.

The getCredentials() call is protected by the UserAdminPermission.GET_CREDENTIAL. Maybe encryption should be left out of this implementation at all, as a result leaving the responsibility to protect the value to the user.

WDYT?

Show
Matthias Küspert added a comment - Yes: that the value type is changed even when not using encryption is definitely a bug. However if values should be encrypted I see no other way than to change the type - otherwise a getCredentials() call would have to return a String with non-ascii characters. Moving the encryption logic to the SPI would IMHO not change this behaviour. The encryption feature is just something I saw in the Felix implementation of the UserAdmin service (https://github.com/apache/felix/tree/trunk/useradmin). It seemed a nice-to-have feature at that time, but I'm not so sure anymore. The getCredentials() call is protected by the UserAdminPermission.GET_CREDENTIAL. Maybe encryption should be left out of this implementation at all, as a result leaving the responsibility to protect the value to the user. WDYT?
Hide
Permalink
Matthias Küspert added a comment -

Regarding 'breaking the contract':

Chapter 107.8.5 states that the contract between the user and the system to put and get values is a Dictionary. Therefore the stored values are of type Object. Nothing is said about the type of objects that go in and out. Therefore, IMHO the implementation is allowed to chage the type if needed.

The spec definitly lacks some detail here. Why not use a Map<>?

However, to keep things simple, the implementation should be changed to use the type of the incoming value when not using encryption. The underlying magic should be documented along with the 'use encryption' switch.

Show
Matthias Küspert added a comment - Regarding 'breaking the contract': Chapter 107.8.5 states that the contract between the user and the system to put and get values is a Dictionary. Therefore the stored values are of type Object. Nothing is said about the type of objects that go in and out. Therefore, IMHO the implementation is allowed to chage the type if needed. The spec definitly lacks some detail here. Why not use a Map<>? However, to keep things simple, the implementation should be changed to use the type of the incoming value when not using encryption. The underlying magic should be documented along with the 'use encryption' switch.
Hide
Permalink
Bram de Kruijff added a comment -

Agreed, spec is inconclusive. I think this is a good compromise. Not sure whether the encryption should be in here at all. Did run into an additional issue.

As the encryption is asynchronously set through cm it may very well be that roles are being created before the useradmin received its updated call. As a result we can end up with a mix of encrypted/unencrypted credentials. Obviously this may also occur when the algorithm is changed along the way, but then you were asking for it I guess.

Show
Bram de Kruijff added a comment - Agreed, spec is inconclusive. I think this is a good compromise. Not sure whether the encryption should be in here at all. Did run into an additional issue. As the encryption is asynchronously set through cm it may very well be that roles are being created before the useradmin received its updated call. As a result we can end up with a mix of encrypted/unencrypted credentials. Obviously this may also occur when the algorithm is changed along the way, but then you were asking for it I guess.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated: