Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add automatic support for p11-kit #260

Merged
merged 8 commits into from
Jun 12, 2023
+85 −27
Merged

Add automatic support for p11-kit #260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2723a8f
Add automatic support for p11-kit
maraino Jun 8, 2023
237a370
Mock findP11KitProxy on New test
maraino Jun 8, 2023
1e80f66
Rely on dlopen to locate p11-kit-proxy.so
maraino Jun 8, 2023
bc3ba29
Fix missing-module test
maraino Jun 8, 2023
a23378d
Add p11-kit-proxy.dll for windows and validate uri
maraino Jun 9, 2023
f840799
Add some docs on the pkcs11 kms
maraino Jun 9, 2023
8d82ef2
Add tests and examples with id with %
maraino Jun 9, 2023
572bfb7
Apply suggestions from code review
maraino Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion kms/pkcs11/pkcs11.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,17 @@ import (
"encoding/hex"
"fmt"
"math/big"
"os"
"os/exec"
"runtime"
"strconv"
"strings"
"sync"
"time"

"github.com/ThalesIgnite/crypto11"
"github.com/pkg/errors"

"go.step.sm/crypto/kms/apiv1"
"go.step.sm/crypto/kms/uri"
)
Expand Down Expand Up @@ -60,7 +66,6 @@ func New(ctx context.Context, opts apiv1.Options) (*PKCS11, error) {
}

config.Pin = u.Pin()
config.Path = u.Get("module-path")
config.TokenLabel = u.Get("token")
config.TokenSerial = u.Get("serial")
if v := u.Get("slot-id"); v != "" {
Expand All @@ -70,6 +75,10 @@ func New(ctx context.Context, opts apiv1.Options) (*PKCS11, error) {
}
config.SlotNumber = &n
}
// Get module or default to use p11-kit-proxy.so
if config.Path = u.Get("module-path"); config.Path == "" {
config.Path = findP11KitProxy(ctx)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the wrong place, isn't it? It'll only happen if a URI is given, but there's an alternative code flow where it's all in the config? That should benefit from the default, too.

There's already a check for config.Path == "" a few lines further down, with an error return which is no longer telling the truth because it says that 'module-path is required'.

Also... what's wrong with pkcs11:manufacturer=piv_II;id=%01 which I usually use as my examples in documentation? Why are only token/serial/slot-id accepted, and not model/manufacturer?

(And why is this something you even need to do for yourself; surely "match token against the attributes in a URI" is a generic thing that ought to be somewhere much further down the stack than your code; in ThalesIgnite/crypto11 or miekg/pkcs11. Ideally you just want to call a function like the matchSlots() I wrote here, and in your case as discussed, you want to just bail if len(slots) != 1).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to see the documentation at least mention that in a properly configured system, module-path should be unnecessary.

In particular, these two:

.. especially the former. I don't want to live in a world where we have to tell users to explicitly pass a module-path.

All those examples for how to use different HSMs differ mostly in the module-path, which shouldn't be needed anyway.

The packaging of the PKCS#11 provider module itself should come with a p11-kit .module file, the token should automatically appear in the output of things like p11-kit list-tokens or p11tool --list-tokens (and thus also in GUIs when you're selecting a client cert for 802.1x or VPN auth, etc.)

The common case is that user should just be able to select a token from the list, e.g. pkcs11:manufacturer=piv_II.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the wrong place, isn't it? It'll only happen if a URI is given, but there's an alternative code flow where it's all in the config? That should benefit from the default, too.

PKCS#11 cannot be configured without a URI, currently you will get the following error:

return nil, errors.New("kms uri 'module-path' are required")

We can make it more explicit and actually fail if URI == "".

Also... what's wrong with pkcs11:manufacturer=piv_II;id=%01 which I usually use as my examples in documentation? Why are only token/serial/slot-id accepted, and not model/manufacturer?

Nothing, but https://github.com/ThalesIgnite/crypto11, which is the one we use on top of github.com/miekg/pkcs11 doesn't support it, see this code.

Regarding the documentation, some of it is managed by a different team, and we haven't integrated this yet, so it will have to wait a bit. But I can change step-kms-plugin README and help once this is integrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed some of this in a23378d

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing, but https://github.com/ThalesIgnite/crypto11, which is the one we use on top of github.com/miekg/pkcs11 doesn't support it, see this code.

Thanks. I've added that specific reference to ThalesGroup/crypto11#104

}
if config.Pin == "" && opts.Pin != "" {
config.Pin = opts.Pin
Expand Down Expand Up @@ -402,4 +411,34 @@ func findCertificate(ctx P11, rawuri string) (*x509.Certificate, error) {
return cert, nil
}

// findP11KitProxy uses pkg-config to locate p11-kit-proxy.so
func findP11KitProxy(ctx context.Context) string {
var out strings.Builder

// It should be more than enough even in constraint VMs
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()

cmd := exec.CommandContext(ctx, "pkg-config", "--variable=proxy_module", "p11-kit-1")
cmd.Stdout = &out
if err := cmd.Run(); err != nil {
return ""
}

path := strings.TrimSpace(out.String())
if _, err := os.Stat(path); err != nil {
if runtime.GOOS != "darwin" {
return ""
}

// pkg-config might return an .so file instead of a .dylib on macOs.
path = strings.Replace(path, ".so", ".dylib", 1)
if _, err := os.Stat(path); err != nil {
return ""
}
}

return path
}

var _ apiv1.CertificateManager = (*PKCS11)(nil)
18 changes: 17 additions & 1 deletion kms/pkcs11/pkcs11_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ func TestNew(t *testing.T) {
return k.p11, nil
}

var (
wantMissingModule *PKCS11
wantErrMissingModule = true
)
if findP11KitProxy(context.Background()) != "" {
wantMissingModule = k
wantErrMissingModule = false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a more accurate test if the output of the shell command were mocked instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use var findP11KitProxy = func(...) {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 237a370


canceledContext, cancel := context.WithCancel(context.Background())
cancel()

type args struct {
ctx context.Context
opts apiv1.Options
Expand Down Expand Up @@ -68,10 +80,14 @@ func TestNew(t *testing.T) {
URI: "pkcs11:module-path=/usr/local/lib/softhsm/libsofthsm2.so;token=pkcs11-test",
Pin: "passowrd",
}}, k, false},
{"fail missing module", args{context.Background(), apiv1.Options{
{"perhaps with missing module", args{context.Background(), apiv1.Options{
Type: "pkcs11",
URI: "pkcs11:token=pkcs11-test",
Pin: "passowrd",
}}, wantMissingModule, wantErrMissingModule},
{"fail findP11KitProxy", args{canceledContext, apiv1.Options{
Type: "pkcs11",
URI: "pkcs11:token=pkcs11-test?pin-value=password",
}}, nil, true},
{"fail missing pin", args{context.Background(), apiv1.Options{
Type: "pkcs11",
Expand Down