KeyClient incorrectly sends explicit null value

[chlowell]

In the REST API spec, the "attributes" object of "KeyItem" isn't marked x-nullable, so a client shouldn't send an explicit null value for this key. KeyClient does, however, which gets an error response from Managed HSM.

repro

#include <azure/identity/azure_cli_credential.hpp>
#include <azure/keyvault/keys/key_client.hpp>
#include <azure/core/http/policies/policy.hpp>
#include <azure/core/http/http.hpp>
#include <iostream>
#include <memory>

class LoggingPolicy : public Azure::Core::Http::Policies::HttpPolicy
{
public:
    std::unique_ptr<Azure::Core::Http::RawResponse> Send(
        Azure::Core::Http::Request &request,
        Azure::Core::Http::Policies::NextHttpPolicy nextHttpPolicy,
        Azure::Core::Context const &context) const override
    {
        std::cout << request.GetMethod().ToString() << " " << request.GetUrl().GetPath() << std::endl;
        if (request.GetBodyStream())
        {
            auto bodyStream = request.GetBodyStream();
            std::vector<uint8_t> body(bodyStream->Length());
            bodyStream->Read(body.data(), body.size(), context);
            bodyStream->Rewind();
            std::cout << "\tBody: " << std::string(body.begin(), body.end()) << std::endl;
        }
        return nextHttpPolicy.Send(request, context);
    }

    std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy> Clone() const override
    {
        return std::make_unique<LoggingPolicy>(*this);
    }
};

int main(int argc, char *argv[])
{
    std::string url = argv[1];

    auto credential = std::make_shared<Azure::Identity::AzureCliCredential>();

    auto opts = Azure::Security::KeyVault::Keys::KeyClientOptions();
    opts.PerRetryPolicies.emplace_back(std::make_unique<LoggingPolicy>());
    Azure::Security::KeyVault::Keys::KeyClient client(url, credential, opts);

    Azure::Security::KeyVault::Keys::CreateRsaKeyOptions createRsaKeyOptions("test-key", true);
    createRsaKeyOptions.KeySize = 2048;
    // uncommenting this enables the request to succeed
    // createRsaKeyOptions.Enabled = true;
    try
    {
        auto key = client.CreateRsaKey(createRsaKeyOptions).Value;
        std::cout << "Created key " << key.Id() << std::endl;
    }
    catch (Azure::Core::RequestFailedException const &e)
    {
        std::cerr << "Error: " << e.Message << std::endl;
    }

    return 0;
}

output

$ ./repro <MHSM URL>
POST /keys/test-key/create?api-version=7.3
        Body: {"attributes":null,"kty":"RSA-HSM"}
Error: Object Attributes: A JSON object was expected (Activity ID: ...)

[LarryOsterman]

QQ: I thought that if a parameter wasn't included in the "required", it was assumed to always be optional and thus nullable (in other words, the semantics are inverted - you have to mark fields and parameters as required rather than marking fields and parameters as optional). Are there other semantics around x-nullable that I'm not aware of?

Having said that, clearly including the "attributes" field is incorrect since it is not present and thus shouldn't be included. And arguably the main KeyVault service should also be rejecting this, not just the MHSM service since it expects that the parameter for "attributes" is always an object.

[chlowell]

My understanding is that "optional" and "nullable" are independent. A field that isn't required is optional in that it needn't appear in a body, but this doesn't make "null" a valid value for that field; "null" is valid only for fields marked x-nullable, regardless of whether they're required.

[ahsonkhan]

That's quite interesting and we sometimes tend to conflate optional and nullable in C++ because of our use of Nullable<T> in Azure::Core to be a stand-in representation for std::optional (only available in C++17 onwards). And we end up marking anything optional as Nullable<T>.

Charles, is the use of x-nullable to indicate "null is a valid value" consistent across all the Azure services and guidelines? I am wondering if we should strictly use the presence of that annotation to drive "null is allowed" decisions. As in, if something isn't marked as such, don't allow null, even if it is marked as optional.

[chlowell]

is the use of x-nullable to indicate "null is a valid value" consistent across all the Azure services and guidelines? I am wondering if we should strictly use the presence of that annotation to drive "null is allowed" decisions. As in, if something isn't marked as such, don't allow null, even if it is marked as optional.

Yes: https://azure.github.io/autorest/extensions/#x-nullable. Nullability is similarly explicit in TypeSpec: https://typespec.io/docs/language-basics/values/#null-values

[ahsonkhan]

In that case, it looks like we'd want to have fields that are optional end up in Options bag (or defaulted signature parameters), but that doesn't immediately imply that they should also be marked as Nullable<T>. I will follow-up to see if there's anything that changes in the generated clients with that stance.

I am not very familiar with the KeyVault spec, could you point to where exactly the corresponding Enabled boolean shows up, that isn't marked as nullable in the spec?

I am trying to see what ends up setting the attributes JSON property to null. The SetFromNullable only sets the property if the value is non-null. https://github.com/Azure/azure-sdk-for-cpp/blob/da92777b8af6aef0dd3e5aabdabbb5da1ee86906/sdk/keyvault/azure-security-keyvault-keys/src/key_request_parameters.cpp#L28-L30

is the use of x-nullable to indicate "null is a valid value" consistent across all the Azure services and guidelines? I am wondering if we should strictly use the presence of that annotation to drive "null is allowed" decisions. As in, if something isn't marked as such, don't allow null, even if it is marked as optional.

Yes: https://azure.github.io/autorest/extensions/#x-nullable. Nullability is similarly explicit in TypeSpec: https://typespec.io/docs/language-basics/values/#null-values

By default, do we assume null is not allowed? The autorest/swagger guideline suggest yes, but curious about TypeSpec:

By default, a null value should be disallowed when forming a request and rejected during payload deserialization.

I see some fields of the KeyAttribute are explicitly marked x-nullable: false. I assume that is redundant? https://github.com/Azure/azure-rest-api-specs/blob/f44ee842a7facb754ae4aa06c78157a0cbb8b30a/specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.3/keys.json#L1473

[ahsonkhan]

The issue is due to this line and the assumptions baked into the calling pattern for JsonOptional::SetFromNullable: https://github.com/Azure/azure-sdk-for-cpp/blob/da92777b8af6aef0dd3e5aabdabbb5da1ee86906/sdk/keyvault/azure-security-keyvault-keys/src/key_request_parameters.cpp#L28-L30

Since the JSON payload doesn't contain the attributes property, at the point of calling that function, when you pass in payload[_detail::AttributesPropertyName] as a function parameter, it cannot find that JSON property, and hence creates a property with a value set to null. This is an implicit/hidden action happening due to how the [] oeprator from nlohmann json works under the covers.

https://github.com/Azure/azure-sdk-for-cpp/blob/da92777b8af6aef0dd3e5aabdabbb5da1ee86906/sdk/core/azure-core/inc/azure/core/internal/json/json.hpp#L25695-L25710

We should instead only be accessing and setting that attributes property on the JSON only after validating that the value we want to set has a value, so that we don't accidentally create a null JSON property as a placeholder, just purely by accessing payload[_detail::AttributesPropertyName].

The same would be true, if we did something like:

Azure::Core::Json::_internal::json payload;
auto temp = payload["foobar"];
// "foobar" doesn't exist within the JSON payload, the mere attempt to access that property creates it and sets to null.
// Now payload contains a "foobar" property set to null.
auto jsonString = payload.dump(); // {"foobar":null}

A solution here is to only access the sub-object, once we know a value exist. Either call it after the if-check (and apply that if-check at every call-site of SetFromNullable):

  if (m_options.Enabled)
  {
    JsonOptional::SetFromNullable(
        m_options.Enabled, payload[_detail::AttributesPropertyName], _detail::EnabledPropertyName);
  }

OR you could consider creating a JsonOptional helper which calls the sub-object only after the if-check on whether a Nullable<T> has a value:

JsonOptional::SetFromNullable(
    m_options.Enabled, payload, _detail::AttributesPropertyName, _detail::EnabledPropertyName);

    template <class T>
    static inline void SetFromNullable(
        Azure::Nullable<T> const& source,
        Azure::Core::Json::_internal::json& jsonKey,
        std::string const& subObject,
        std::string const& keyName)
    {
      if (source)
      {
        jsonKey[subObject][keyName] = source.Value();
      }
    }

A coding pattern we should review and avoid is one where a nlohmann json object's [] operator is being used as part of the calling parameter, since it has this unintended side-effect of mutating the JSON object.

cc @antkmsft since this could impact code generation and JSON serialization patterns