smartcard handler for request authentication and CSR generation #34

py4chen · 2023-01-03T11:08:29Z

PR defines a smartcard handler to authenticate and generate CSRs for yubikey-backed SSH certificates. PR also includes 3 authentication modules and 1 csr module, which can be plugged into the smartcard handler by setting the ysshra configuration.

A sample smartcard handler config will be like:

"handlers":
    {
        "smartcard":
        {
            "authn": [
                {
                    "module": "f9_verify",
                    "f9_certs_dir": "/opt/ysshra/f9_certs/"
                },
                {
                    "module": "slot_attest",
                    "slot": "9a",
                    "piv_root_ca": "/opt/ysshra/piv_root_ca.pem",
                    "u2f_root_ca": "/opt/ysshra/u2f_root_ca.pem",
                },
                {
                    "module": "slot_attest",
                    "slot": "9e",
                    "piv_root_ca": "/opt/ysshra/piv_root_ca.pem",
                    "u2f_root_ca": "/opt/ysshra/u2f_root_ca.pem",
                },
                {
                    "module": "slot_serial",
                    "slot": "9e",
                    "yubikey_mappings": "/opt/ysshra/yubikey_mappings",
                },
            ],
            "csr":  [
                {
                    "module": "smartcard_hardkey",
                    "_comment": "touchSSH",
                    "is_firefighter": false,
                    "touch_policy": 3,
                    "principals": "<logname>,<logname>:touch",
                    "slot": "9e",
                    "cert_validity_sec": 86400
                }
            ],
            "key_identifiers": {
                "default": "ssh-user-key",
            }
        }
    },

Notes:

There are no unit tests for module slot_attest, which will be covered in another PR.

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

agent/yubiagent/generate_test.go

agent/yubiagent/slot.go

adavis10006 · 2023-01-07T06:41:57Z

agent/yubiagent/slot.go

+}
+
+// RegisterCSR registers the given certificate signing request for the key slot.
+func (s *SlotAgent) RegisterCSR(csr *proto.SSHCertificateSigningRequest) {


I'm not quite sure why we need to keep the CSR. Can you provide use cases?

We need both CSR and AddCertsToAgent in struct slot, because the struct implements the interface csr.AgentKey. In this way, gensign main logic could extract all the CSRs for a key pair (or a key slot), sign them, and add the signed certificates to the corresponding agent.

for _, agentKey := range csrAgentKeys { var ( certs []ssh.PublicKey comments []string ) for _, csr := range agentKey.CSRs() { cert, comment, err := signer.Sign(ctx, csr) if err != nil { return fmt.Errorf("failed to sign CSR: %v", err) } certs = append(certs, cert...) comments = append(comments, comment...) } err = agentKey.AddCertsToAgent(certs, comments) if err != nil { return fmt.Errorf("failed to add certificates into the agent: %v", err) } }

To me, it feels like we probably need to review the design. Now, it is more like we need those methods exported because the implementation requires this to do so, not the methods is available to the struct or object.
For more specific, an Yubikey PIV slot can contain private key, and certificate but not CSRs. It doesn't make sense the retrieve it from the agent just because the implementation needs it. Similar, the ssh-agent doesn't hold the CSRs.
Also, allow a slot to retrieve its agent might be helpful in implementation, but doesn't make sense to expose it.

Checking the Handler interface
It probably will make more sense to just pass the csr.Signer into the handler to sign the certificate and store into the agent.

Per discussion, the new interface will be like:

type Handler interface { Name() string Authenticate(params *csr.ReqParam) error Sign(signer, param) error // 1. Gen CSR, 2. Sign, 3. Add into agent }

adavis10006 · 2023-01-07T06:42:35Z

agent/yubiagent/slot.go

+}
+
+// Agent returns the yubiagent.
+func (s *SlotAgent) Agent() YubiAgent {


Why we want to export this? Can you provide use cases?

There are 2 use cases:

During ssh key challenge, we will need the agent of that key slot to invoke "Sign()"
https://github.com/theparanoids/ysshra/blob/main/agent/ssh/agent.go#L74-L85

During attestation, I want to use the same agent of that key slot to fetch the f9 certificate.

f9Cert, err := a.slot.Agent().ReadSlot("f9")

For (2), it could be eliminated by creating a slot object for f9. For (1), I don't like to expose another function Sign() for struct Slot. Thus, I expose Agent()

This doesn't sound right. It make more sense to implement Sign() for a Slot and for the f9, it should be hold inside the struct

type authn struct { attest *yubiagnet.Slot slotAgent *yubiagent.Slot conf *conf }

agent/yubiagent/slot.go

config/module.go

modules/authn_slot_attest/conf.go

modules/authn_slot_serial/conf.go

modules/csr_smartcard_hardkey/conf.go

maditya · 2023-01-09T11:48:03Z

modules/authn_f9_verify/authn.go

+	// but now it needs 8 decimal digits.
+	// For those old yubikeys with serial numbers of 7 decimal digits,
+	// the file name of the corresponding f9 cert is prepended with a `0`.
+	if len(f9SerialNumStr) < 8 {


can we prepend 8 - len(f9SerialNumStr) 0s instead of assuming the old ones to be 7 digit long and only prepending one 0?

maditya · 2023-01-09T12:41:08Z

agent/yubiagent/slot.go

+		default:
+			return false
+		}
+	// ed25519 is non-supported key algo for now.


ed25519 is supported by yubikey but ysshra doesn't yet support it, correct? If so, can we add a todo here?

According to https://support.yubico.com/hc/en-us/articles/360013647820-YubiKey-4C, it is not supported in PIV mode.

maditya · 2023-01-09T12:41:34Z

agent/yubiagent/slot.go

+		switch pk.Size() * 8 {
+		case 2048:
+			return true
+		default:


any reason RSA 4096 and 8192 is not supported?

According to https://support.yubico.com/hc/en-us/articles/360013647820-YubiKey-4C, it is not supported in PIV mode.

maditya · 2023-01-09T12:44:55Z

agent/yubiagent/slot.go

+		// NOTE: The following id is the touch policy stored in attestation certificate.
+		//       Refer: https://developers.yubico.com/PIV/Introduction/PIV_attestation.html
+		if ext.Id.String() == "1.3.6.1.4.1.41482.3.8" {
+			touch = keyid.TouchPolicy(ext.Value[1])


need a nil check for ext.Value ?

maditya · 2023-01-09T12:47:03Z

modules/authn_f9_verify/authn.go

+
+// VerifyF9Cert will ensure that the user is using a Yubikey that was provisioned to him or her,
+// rather than just any Yubikey.
+func VerifyF9Cert(f9CertDirPath string, f9Cert *x509.Certificate) error {


does it need to be public?

maditya · 2023-01-09T12:52:43Z

modules/authn_f9_verify/authn.go

+	// Name is a unique name to identify an authentication module.
+	Name = "f9_verify"
+
+	modHexMap = "cbdefghijklnrtuv"


nit: modHexMap is defined twice in two different packages.

py4chen added 5 commits January 3, 2023 19:07

add modules and tests

5ca124f

add more tests

18e753e

add more tests

1c76728

update comments and vars

d487065

fix tests

bf84c5e

py4chen marked this pull request as ready for review January 5, 2023 10:59

py4chen requested review from adavis10006, hkadakia, maditya and zhaoyonghe January 5, 2023 11:08

py4chen changed the title ~~[DNR] smartcard authn & csr modules~~ smartcard handler for request authentication and CSR generation Jan 5, 2023

py4chen commented Jan 5, 2023

View reviewed changes

agent/yubiagent/generate_test.go Outdated Show resolved Hide resolved

adavis10006 reviewed Jan 7, 2023

View reviewed changes

py4chen added 2 commits January 9, 2023 16:04

address comments

5e1f7b0

add mock package

e10eea7

py4chen requested a review from adavis10006 January 9, 2023 08:38

maditya reviewed Jan 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

smartcard handler for request authentication and CSR generation #34

smartcard handler for request authentication and CSR generation #34

py4chen commented Jan 3, 2023 •

edited

Loading

adavis10006 Jan 7, 2023

py4chen Jan 9, 2023

py4chen Jan 9, 2023 •

edited

Loading

adavis10006 Jan 10, 2023

adavis10006 Jan 10, 2023

py4chen Jan 10, 2023

adavis10006 Jan 7, 2023

py4chen Jan 9, 2023 •

edited

Loading

adavis10006 Jan 10, 2023 •

edited

Loading

maditya Jan 9, 2023

maditya Jan 9, 2023

adavis10006 Jan 10, 2023

maditya Jan 9, 2023

adavis10006 Jan 10, 2023

maditya Jan 9, 2023

maditya Jan 9, 2023

maditya Jan 9, 2023

smartcard handler for request authentication and CSR generation #34

Are you sure you want to change the base?

smartcard handler for request authentication and CSR generation #34

Conversation

py4chen commented Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

py4chen Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

py4chen Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

adavis10006 Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

py4chen commented Jan 3, 2023 •

edited

Loading

py4chen Jan 9, 2023 •

edited

Loading

py4chen Jan 9, 2023 •

edited

Loading

adavis10006 Jan 10, 2023 •

edited

Loading