Should Key have always an expiring date ?

[juchom]

While I was working on updating a key in the C# client, I faced an issue with current design for expiring date for keys.

meilisearch/meilisearch-dotnet#245 (comment)

Actually if a key has a date it will expire at this date. If it's null, the key will never expire.

The doc for updating a key says :

Updates to keys are partial. This means you should provide only the fields you intend to update, as any fields not present in the payload will remain unchanged.

In strongly typed language JsonSerializer can be configured to remove null values from objects or to keep it. Which can lead to errors for your customers if we try to create a client that sticks to your api

Imagine we have this object for our call to the api

public class UpdateKeyInfo
{
    public string Id { get; set; }
    public List<string> Actions { get; set; }
    public DateTime? ExpiresAt { get; set; }
}

If the JsonSerializer removes null

If a user creates an instance of this object because he wants to only change the Actions list but not the expiring date, the call will do what the user wants.

If a user wants to have a key that never expires and set null to this object, the value won't be sent.

If the JsonSerializer keeps null

If a user creates an instance of this object because he wants to only change the Actions list but not the expiring date, the call will also set the value null to the expiring date which is not expected.

If a user wants to have a key that never expires and set null to this object and leave null for Actions, the value for the date will be sent but the a null list of actions will be sent also.

I think, it would be better to always have a value for the expiring date. If you want it to expires in one year from now set it to one year from now, if you want it to never expires set the value to 9999-12-31T23:59:59.9999999 or something very far in the future.

In Azure Portal they manage keys like that, a key that never expires is set to expires in a century or something like that.

Doing so, would make it predicable for users. This will follow the same rule for every fields : If it's null don't send it, if it's not send it.

But in any case, you have to set a value when you create the key or if you want to change the expiry date.

[gmourier]

Hi @juchom 👋

Thanks for this feedback.

I think, it would be better to always have a value for the expiring date. If you want it to expires in one year from now set it to one year from now, if you want it to never expires set the value to 9999-12-31T23:59:59.9999999 or something very far in the future.

IMO, it can be counter-intuitive regarding the usage of an API. It forces users to set a value while they don't necessarily want to. It adds another step during their workflow while a configured serializer seems to solve the need to match the behavior of an API using the null marker to define a property. 🤔

We thought about forcing an expiresAt for security reasons but decided not to go in that direction as it may be better for the developer in terms of experience when testing and configuring Meilisearch during the development cycle. Nevertheless, we have chosen to use the null marker so that the user must explicitly define a non-expiring key if he makes this choice.

Which can lead to errors for your customers if we try to create a client that sticks to your API

Can you elaborate on this? Are you speaking about a front-end client or a custom SDK? 😇

Thanks!

[juchom]

It's C# code, if something is not clear do not hesitate to ask

To update a key the doc says this :

Updates to keys are partial. This means you should provide only the fields you intend to update, as any fields not present in the payload will remain unchanged.

In a strongly typed language, the contract for updating an keep would be :

public class UpdateKey
{
	public string Key { get; set; }
	public string Description { get; set; }
	public List<string> Actions { get; set; }
	public List<string> Indexes { get; set; }
	public DateTime? ExpiresAt { get; set; }
}

In our code we can tell the JsonSerializer to remove or keep null values, here is our two configurations

var removeNulls = new JsonSerializerOptions
{
	DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
	PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
};

var keepNulls = new JsonSerializerOptions
{
	DefaultIgnoreCondition = JsonIgnoreCondition.Never,
	PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
};

Imagine we want to update our key and remove the expiry :

var key = new UpdateKey
{
	Key = "d0552b41536279a0ad88bd595327b96f01176a60c2243e906c52ac02375f9bc4",
	Actions = new List<string> { "search", "documents.get" },
	ExpiresAt = null,
};

If we serialize this object with removing null values we will have this json

{
  "key": "d0552b41536279a0ad88bd595327b96f01176a60c2243e906c52ac02375f9bc4",
  "actions": [
    "search",
    "documents.get"
  ]
}

As you can see the payload will miss the ExpiresAt because it is null.

If we keep null values we will have this json

{
  "key": "d0552b41536279a0ad88bd595327b96f01176a60c2243e906c52ac02375f9bc4",
  "description": null,
  "actions": [
    "search",
    "documents.get"
  ],
  "indexes": null,
  "expiresAt": null
}

In this case we will keep all values but we will overwrite description and indexes.

This means that null values are to be excluded for everything but for the expiresAt property the null value has a real meaning and should not be ignored.

I think it's better to always have an expiry date, a user may not always read the documentation and could create a key that never expires which may not be what he wanted or may create a security hole.

Having expiresAt mandatory at the creation makes clear for everybody when the key will expire and if you don't want to expire set this date very far in the future.

[juchom]

I was thinking at this problem and given the actual definition of a key, you are right setting a date far in the future to say it's not expiring is wrong.

I think the main issue is to give the possibility to create a key that never expires for something that contains access rights.

Since you want to offer Meilisearch as a hosted service, I think for a cloud service it has to be clear that the expiry date is a mandatory attribute of your keys. Just let your customers decide what they want, they want keys that expires every month or in a century let them decide.

I let you decide if you want to change the definition of your access keys to always expiring, and make the expiry date field mandatory.

Again, since you want to offer Meilisearch as a service, you should definitely consider having to access key per key to allow your customers rotating their access keys without service interruption.

Let me know what you decide and I will update the .net client accordingly.

[gmourier]

Hi @juchom 👋

We had a working group about this with @bidoubiwa and @brunoocasali today.

We'll make expiresAt to have an expiration date defined in any case, we believe it's the most obvious and simplest solution to avoid this problem.

Also, it's more secure by design which is a good thing in the long run. A key that never expires can be problematic and users can always set a date far into the future as a work-around.

This change is planned for v0.28.

Thanks a lot for your help on this subject and for bringing up that issue to the table. ❤️

[juchom]

Ok, this is a good news !

While you are working on this topic, even if this is not linked to our issue, will you add 2 access keys per key to help in rotating keys ?

And thanks for making this great product ! 👍

[gmourier]

I have created another product discussion so that we can discuss it specifically and make it easier for others to contribute.

#438

[gmourier]

Hey 👋

A quick update; keys can still have no expiration date if desired (as currently, it is necessary to specify null at creation).

The change for v0.28 is that the security-related fields (actions/ indexes and expiresAt) can no longer be modified for an API key.

[gmourier]

Following the v0.28 release, this discussion is now locked.

We encourage you to create a new thread if needed.

Thank you ✨