Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ jobs:

steps:
- name: Checkout repository
uses: actions/checkout@v3
uses: actions/checkout@v4

# Initializes the CodeQL tools for scanning.
- name: Initialize CodeQL
uses: github/codeql-action/init@v1
uses: github/codeql-action/init@v3
with:
languages: ${{ matrix.language }}
# If you wish to specify custom queries, you can do so here or in a config file.
Expand All @@ -54,7 +54,7 @@ jobs:
# Autobuild attempts to build any compiled languages (C/C++, C#, or Java).
# If this step fails, then you should remove it and run the build manually (see below)
- name: Autobuild
uses: github/codeql-action/autobuild@v1
uses: github/codeql-action/autobuild@v3

# ℹ️ Command-line programs to run using the OS shell.
# 📚 https://git.io/JvXDl
Expand All @@ -68,4 +68,4 @@ jobs:
# make release

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v1
uses: github/codeql-action/analyze@v3
13 changes: 8 additions & 5 deletions .github/workflows/golangci-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@ jobs:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/setup-go@v3
- uses: actions/checkout@v4

- uses: actions/setup-go@v5
with:
go-version: 1.19
- uses: actions/checkout@v3
go-version-file: ./go.mod


- name: golangci-lint
uses: golangci/golangci-lint-action@v3
uses: golangci/golangci-lint-action@v6
with:
# Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
version: v1.49
version: latest

# Optional: working directory, useful for monorepos
# working-directory: somedir
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/update-dependency.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Git Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Install Go
uses: actions/setup-go@v3
uses: actions/setup-go@v5
with:
go-version: '1.19.x'
go-version-file: ./go.mod

- name: Install Ziti CI
uses: openziti/ziti-ci@v1
Expand Down
13 changes: 5 additions & 8 deletions engines/parsec/parsec_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ package parsec

import (
"crypto"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/ecdh"
"encoding/asn1"
"github.com/michaelquigley/pfxlog"
"github.com/openziti/identity/engines"
Expand Down Expand Up @@ -48,17 +47,15 @@ func (e *engine) LoadKey(key *url.URL) (crypto.PrivateKey, error) {
if err != nil {
return nil, err
}
x, y := elliptic.Unmarshal(elliptic.P256(), pubBytes)
pubKey, err := ecdh.P256().NewPublicKey(pubBytes)

pub := &ecdsa.PublicKey{
Curve: elliptic.P256(),
X: x,
Y: y,
if err != nil {
return nil, err
}

return &parsecKey{
name: keyName,
pub: pub,
pub: pubKey,
}, nil
}

Expand Down
18 changes: 9 additions & 9 deletions engines/pkcs11/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,28 +84,28 @@ func Test_softhsm2_keys(t *testing.T) {
if err != nil {
t.Error(err)
} else {
test_signer(key, t)
testSigner(key, t)
}
}
}

func test_signer(key crypto.PrivateKey, t *testing.T) {
priv, ok := key.(crypto.Signer)
func testSigner(key crypto.PrivateKey, t *testing.T) {
privateKey, ok := key.(crypto.Signer)
if !ok {
t.Error("key is not a crypto.Signer")
}

pub := priv.Public()
pub := privateKey.Public()

bytes := make([]byte, 32)
_, _ = rand.Read(bytes)

sig, err := priv.Sign(rand.Reader, bytes, crypto.SHA256)
sig, err := privateKey.Sign(rand.Reader, bytes, crypto.SHA256)
if err != nil {
t.Error(err)
}

switch pubkey := pub.(type) {
switch pubKey := pub.(type) {
case *ecdsa.PublicKey:
var ecSig ecdsaSig
rest, err := asn1.Unmarshal(sig, &ecSig)
Expand All @@ -116,15 +116,15 @@ func test_signer(key crypto.PrivateKey, t *testing.T) {
t.Errorf("leftover bytes")
}

cool := ecdsa.Verify(pubkey, bytes, ecSig.R, ecSig.S)
cool := ecdsa.Verify(pubKey, bytes, ecSig.R, ecSig.S)
if !cool {
t.Errorf("signature validation fail")
}

case *rsa.PublicKey:
err = rsa.VerifyPKCS1v15(pubkey, crypto.SHA256, bytes, sig)
err = rsa.VerifyPKCS1v15(pubKey, crypto.SHA256, bytes, sig)
if err != nil {
t.Errorf(err.Error())
t.Error(err.Error())
}

default:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/openziti/identity

go 1.19
go 1.23

require (
github.com/fsnotify/fsnotify v1.7.0
Expand Down
96 changes: 88 additions & 8 deletions identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,15 @@ type Identity interface {
// StopWatchingFiles reversed WatchFiles.
StopWatchingFiles()

// IsCertSettable returns nil if the "cert" certificate storage supports writing, used before calling SetCert()
IsCertSettable() error
Copy link
Member

Choose a reason for hiding this comment

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

imo, it seems strange for an 'Is' function to return error not a bool

Copy link
Member Author

@andrewpmartinez andrewpmartinez Apr 10, 2025

Choose a reason for hiding this comment

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

It is a go thing because errors are returned, not raised.

Consider this set of outcomes: for (bool,error) function return values:

1: true, <nil>
2: false, err
3: false, <nil>

It forces the caller to inspect two values instead of 1. By returning error only, they only have to inspect 1. Additionally, nothing stops the code from returning true, error and in which case, what does that mean?

Returning just bool doesn't allow error propagation for other issues.

Copy link
Member

Choose a reason for hiding this comment

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

:D if you say so. I'd just prefer the "Is" to be "Verify" or "Check" or literally anything that isn't "Is" but i don't care all that much


// SetCert updates the current client cert in use and saves it to the identity file.
SetCert(pem string) error

// IsServerCertSettable returns nil if the server certificate storage supports writing, used before calling SetServerCert()
IsServerCertSettable() error

// SetServerCert update the current server cert in use and saves it to the identity file.
SetServerCert(pem string) error

Expand Down Expand Up @@ -276,6 +282,66 @@ func (id *ID) initCert(loadedCerts []*x509.Certificate) error {
return nil
}

func (id *ID) IsCertSettable() error {
certUrl, err := parseAddr(id.Config.Cert)

if err != nil {
return err
}

switch certUrl.Scheme {
case StoragePem:
return errors.New("cannot save cert in pem storage format")
case StorageFile, "":
absPath, err := filepath.Abs(id.Config.Cert)

if err != nil {
return fmt.Errorf("cannot get absolute path for cert file %s: %w", id.Config.Cert, err)
}

f, err := os.OpenFile(absPath, os.O_RDWR, 0664)

if err != nil {
return fmt.Errorf("can not save cert certificate [%s] due to file error: %v", absPath, err)
}
defer func() { _ = f.Close() }()

return nil
}

return fmt.Errorf("can not save cert certificate, location scheme not supported (%s) or address not defined (%s)", certUrl.Scheme, id.Config.Cert)
}

func (id *ID) IsServerCertSettable() error {
certUrl, err := parseAddr(id.Config.ServerCert)

if err != nil {
return err
}

switch certUrl.Scheme {
case StoragePem:
return errors.New("cannot save server cert in pem storage format")
case StorageFile, "":
absPath, err := filepath.Abs(id.Config.ServerCert)

if err != nil {
return fmt.Errorf("cannot get absolute path for server cert file %s: %w", id.Config.ServerCert, err)
}

f, err := os.OpenFile(absPath, os.O_RDWR, 0664)

if err != nil {
return fmt.Errorf("can not save server certificate [%s] due to file error: %v", absPath, err)
}
defer func() { _ = f.Close() }()

return nil
}

return fmt.Errorf("can not save server certificate, location scheme not supported (%s) or address not defined (%s)", certUrl.Scheme, id.Config.ServerCert)
}

// SetCert persists a new PEM as the ID's client certificate.
func (id *ID) SetCert(pemStr string) error {
certUrl, err := parseAddr(id.Config.Cert)
Expand All @@ -287,28 +353,35 @@ func (id *ID) SetCert(pemStr string) error {
switch certUrl.Scheme {
case StoragePem:
id.Config.Cert = StoragePem + ":" + pemStr
return fmt.Errorf("could not save client certificate, location scheme not supported for saving (%s):\n%s", id.Config.Cert, pemStr)
return fmt.Errorf("could not save cert certificate, location scheme not supported for saving (%s):\n%s", id.Config.Cert, pemStr)
case StorageFile, "":
f, err := os.OpenFile(id.Config.Cert, os.O_RDWR, 0664)

absPath, err := filepath.Abs(id.Config.Cert)

if err != nil {
return fmt.Errorf("cannot get absolute path for cert file %s: %w", id.Config.Cert, err)
}

f, err := os.OpenFile(absPath, os.O_RDWR, 0664)
if err != nil {
return fmt.Errorf("could not update client certificate [%s]: %v", id.Config.Cert, err)
return fmt.Errorf("could not update cert certificate [%s]: %v", id.Config.Cert, err)
}

defer func() { _ = f.Close() }()

err = f.Truncate(0)

if err != nil {
return fmt.Errorf("could not truncate client certificate [%s]: %v", id.Config.Cert, err)
return fmt.Errorf("could not truncate cert certificate [%s]: %v", id.Config.Cert, err)
}

_, err = fmt.Fprint(f, pemStr)

if err != nil {
return fmt.Errorf("error writing new client certificate [%s]: %v", id.Config.Cert, err)
return fmt.Errorf("error writing new cert certificate [%s]: %v", id.Config.Cert, err)
}
default:
return fmt.Errorf("could not save client certificate, location scheme not supported (%s) or address not defined (%s):\n%s", certUrl.Scheme, id.Config.Cert, pemStr)
return fmt.Errorf("could not save cert certificate, location scheme not supported (%s) or address not defined (%s):\n%s", certUrl.Scheme, id.Config.Cert, pemStr)
}

return nil
Expand All @@ -324,9 +397,16 @@ func (id *ID) SetServerCert(pem string) error {
switch certUrl.Scheme {
case StoragePem:
id.Config.ServerCert = StoragePem + ":" + pem
return fmt.Errorf("could not save client certificate, location scheme not supported for saving (%s): \n %s", id.Config.Cert, pem)
return fmt.Errorf("could not save server certificate, location scheme not supported for saving (%s): \n %s", id.Config.ServerKey, pem)
case StorageFile, "":
f, err := os.OpenFile(id.Config.ServerCert, os.O_RDWR, 0664)

absPath, err := filepath.Abs(id.Config.ServerCert)

if err != nil {
return fmt.Errorf("cannot get absolute path for server cert file %s: %w", id.Config.ServerCert, err)
}

f, err := os.OpenFile(absPath, os.O_RDWR, 0664)
if err != nil {
return fmt.Errorf("could not update server certificate [%s]: %v", id.Config.ServerCert, err)
}
Expand Down
2 changes: 2 additions & 0 deletions token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ type mockIdentity struct {
clientCert *tls.Certificate
}

func (m *mockIdentity) IsCertSettable() error { return nil }
func (m *mockIdentity) IsServerCertSettable() error { return nil }
func (m *mockIdentity) GetX509ActiveClientCertChain() []*x509.Certificate { return nil }
func (m *mockIdentity) GetX509ActiveServerCertChains() [][]*x509.Certificate { return nil }
func (m *mockIdentity) GetX509IdentityServerCertChain() []*x509.Certificate { return nil }
Expand Down