AntelopeIO / leap

C++ implementation of the Antelope protocol
Other
114 stars 70 forks source link

Support rpId registrable domain suffix from origin - WebAuthN #1409

Open AndlerRL opened 1 year ago

AndlerRL commented 1 year ago

Hello all! I hope I get some help in here. Currently I have an application that creates WA key types which we have been using for a whille. According to WebAuthN security rules, we can set a Relying Party Identificator rp.id to be equal to the domain instead of the origin if we specify it on the navigator.credentials.create() function on the Credential Manager API, as well the navigator.credentials.get(). The key is being generated correctly with no issues but it started to fail whenever we try to sign an action with it. It looks that it complains about the origin is not the same as the rpId, so by checking the error origin: elliptic_webauthn.cpp:244 https://github.com/AntelopeIO/leap/blob/main/libraries/libfc/src/crypto/elliptic_webauthn.cpp#L244 I found that we are checking on a full match but, shouldn't this be attempting to check if this origin at least has the domain on the rpId? For example:

If example.com is the rp.id but the origin is sub.example.com, then this should be true (as for WebAuthN too) hence, to check this, we loop throughout the subdomains and domain if they match. If not, then we return that the domain does not match with rpId. Currently that it is not happening.

I really would like to have this feature improvement on the protocol, so new accounts can use their keys on different subdomains and within the same domain.

Additonal Details:

Functions that creates the key and signatures:

I'm using @greymass/webauthn package on my front-end application to create signatures and public key from the client. I made a local file on my application based on this to make local debug and test.:

Code Change Proposal:

In theory, what we need to do is to split all domain suffix from origin and verify if this matches with the rpId hash. I made some changes on the code locally for the v3.1.2 release but I think it can be implemented on the next version release. On the next code that I'll share, notice that I might be missing some parts of the code that changes the rpId that we need to verify and that is what I'm attempting to do here. To see a way of how we can validate the suffixes from origin and check it against the rpId hash if it matches:

#include <boost/algorithm/string.hpp>

// ...
public_key::public_key(const signature& c, const fc::sha256& digest, bool) {
  // ...
   char required_origin_scheme[] = "https://";
   size_t https_len = strlen(required_origin_scheme);
   FC_ASSERT(handler.found_origin.compare(0, https_len, required_origin_scheme) == 0, "webauthn origin must begin with https://");
   std::string origin_host = handler.found_origin.substr(https_len, handler.found_origin.rfind(':') - https_len);
   rpid = origin_host;

   // ...

   static_assert(min_auth_data_size >= sizeof(fc::sha256), "auth_data min size not enough to store a sha256");

   std::vector<std::string> origin_host_parts;
   boost::split(origin_host_parts, origin_host, boost::is_any_of("."));

   std::string rpid_check = rpid;
   bool is_suffix = false;

   for (int i = origin_host_parts.parts.size() - 1; 1 >= 0 && !rpid_check.empty(); --i) {
      std::string host_suffix = origin_host_parts[i] + (i < origin_host_parts.size() - 1 ? "." : "") + host_suffix;

      if (host_suffix == rpid_check) {
         is_suffix = true;
         break;
      }
   }

   FC_ASSERT(is_suffix, "webauthn origin must be a suffix of the rpid");

   // replace rpid with the suffix

   FC_ASSERT(memcmp(c.auth_data.data(), fc::sha256::hash(rpid).data(), sizeof(fc::sha256)) == 0, "webauthn rpid hash doesn't match origin");

  // ...
}

I feel that the last line on this should be removed or to replace the rpId to the suffix or to pass this verification to a variable memcmp(c.auth_data.data(), fc::sha256::hash(rpid).data(), sizeof(fc::sha256)) == 0 to either loop it on the for loop to check if none of it is a true case (as doing on the is_suffix variable on the for loop.

Relaying Party Identificator - WebAuthN 📚

Specification of the rp.id in case needed, pay attention to this details

bhazzard commented 11 months ago

The idea and request are sound, however its hard for us to justify as a priority for the core team to tackle currently.

In addition to making code enhancements, the effort involved to implement this would require a protocol change because it affects signature validation.

I'd like to better understand the use cases and level of usage around this feature.

AndlerRL commented 11 months ago

Sure, no problem @bhazzard ! There are some points to consider this feature for the EOS ecosystem in general:

  1. WebAuthN Standards: WebAuthN Spec states Relying Party Identifier ( rpid ) should support subdomains. Antelope Leap does not support it, we have identified the lib line that requires modification. This line only supports a fixed rp.id, in order to support subdomains this validation needs to be updated to follow the spec.
  2. Highly Mainstream Usage: Biometric/PIN/FaceID access is turning to mainstream and we have found that at least ~200 users (of ~220) in our dApp found it very useful. The app recently launched and it has progressively grown with this feature built into our dApp. It won't take too much to see that many users would prefer to have custody of their accounts using their devices as the authenticator verifier and it is on us to provide the future-ready from what is coming next.
  3. Business Reasons: Currently there are not many applications that support and provide such functionality. This is because it has some drawbacks in terms of utility and blockchain costs. Currently, if a dApp creates accounts with this feature and this business wants to keep the user keys to be used among all the business products, it can't, because they need to create separate accounts to be able to use its keys on that website and this business needs to acquire multiple domains for the accounts to be trackable among products, for example:

    bitcash.org has the main dApp on the website app.bitcash.org, which will have different products such as smart.bitcash.org, dboard.bitcash.org, etc... by having different subdomains, Antelope would force us to create multiple accounts technically with the same ownership and person who custody the account, making the blockchain bigger and bigger without the necessity of doing so.

In short, businesses cannot implement features that are native from WebAuthN because we do not support them as currently is hence, new/current business ideas would not happen because of these limits as well it will feel that we are behind and that could give that business the possibility to change direction in relation with Antelope ecosystem usage.

merivercap commented 11 months ago

Hey @bhazzard, Founder of bitcash here.

This update is very important to us and the EOS network. Strategic Value & Future-Proofing: Crypto ecosystems are evolving rapidly. While it's essential to address immediate priorities, it's equally important to preemptively adapt to anticipated shifts in the landscape. By enhancing our protocol, especially in areas like signature validation, we solidify our position as a forward-thinking platform. This ensures we aren't caught on the back foot when industry standards evolve or when user expectations shift.

User Demand & Adoption: The use-cases around this feature aren't just theoretical. Webauthn will be the KEY feature to get crypto mainstream and incredibly important for the entire crypto industry. We can be in the forefront. Implementing this feature could be pivotal in driving user adoption or retaining our existing user base. While in our beta phase we have 200+ users but our goal is to scale that to the tens of thousands of users very soon and this update is pivotal.

WebAuthN Standards: -WebAuthN's Relying Party Identifier (rpid) should support subdomains. -Antelope Leap doesn't align with this spec due to a specific lib line. -To comply, this validation should be updated.

Highly Mainstream Usage: -Biometric/PIN/FaceID access is becoming mainstream. -A significant trend: users prefer device-based authenticator verifiers.

Business Reasons: -Few applications currently support such features due to utility and blockchain cost constraints. -If a dApp integrates this feature, current structure restricts seamless key usage across business products. Example: bitcash.org would require multiple accounts for subdomains like app.bitcash.org, smart.bitcash.org, etc. This results in unnecessary blockchain bloat.

Business Impact: -Current limitations deter businesses from implementing native WebAuthN features. -Could discourage businesses from engaging with the Antelope ecosystem, feeling it's outdated.

Thanks and hope you can push this through!