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.

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

linter #18

Closed
wants to merge 1 commit into from
Closed
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
6 changes: 5 additions & 1 deletion internal/app/agent/readiness_checker/readiness_checker.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package readiness_checker

//TODO: io/ioutil is deprecated, replace to fs.FS
import (
"context"
"errors"
Expand Down Expand Up @@ -154,7 +156,8 @@ func serializeReport(r *protoagent.AgentReadinessReport) ([]byte, error) {
}

func openLogFile(path string) (*os.File, error) {
f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o644)
// #nosec G304
f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600)
if err != nil {
return nil, fmt.Errorf("failed to open the file \"%s\": %w", path, err)
}
Expand All @@ -166,6 +169,7 @@ func ReadReport(ctx context.Context, logDir string) (*protoagent.AgentReadinessR
if err != nil {
return nil, err
}
// #nosec G304
fp, err := os.Open(logFilePath)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
Expand Down
2 changes: 2 additions & 0 deletions internal/app/agent/upgrader/starter/files/starter.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package files

//TODO: io/ioutil is deprecated, replace to fs.FS and delete "nolint:staticcheck"
import (
"fmt"
"io/ioutil"
Expand Down
1 change: 1 addition & 0 deletions internal/app/agent/upgrader/upgrader.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ func (u *upgrader) replaceAgentExecutableWithUpgrader(_ context.Context) error {

func (u *upgrader) startAgentProcess(_ context.Context) (int, error) {
args := getAgentProcessFlags()
// #nosec G204
cmd := exec.Command(u.conf.AgentExecutablePath, args...)
if err := cmd.Start(); err != nil {
return -1, fmt.Errorf("failed to start the new agent process: %w", err)
Expand Down
6 changes: 5 additions & 1 deletion internal/app/agent/utils/lock_file.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package utils

//TODO: io/ioutil is deprecated, replace to fs.FS and delete "nolint:staticcheck"
import (
"context"
"errors"
Expand All @@ -13,7 +15,8 @@ import (
)

func CreateLockFile(name string) error {
f, err := os.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o666)
// #nosec G304
f, err := os.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600)
if err != nil {
return fmt.Errorf("failed to create a lock file: %w", err)
}
Expand Down Expand Up @@ -52,6 +55,7 @@ func WatchLockFile(ctx context.Context, name string) error {
}

func getPID(name string) (int, error) {
// #nosec G304
pidBytes, err := ioutil.ReadFile(name)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
Expand Down
4 changes: 3 additions & 1 deletion internal/app/agent/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ func checkFileExists(path string) error {
}

func copyContents(src string, dst string, mode os.FileMode) error {
// #nosec G304
srcF, err := os.Open(src)
if err != nil {
return fmt.Errorf("failed to open the source file %s: %w", src, err)
Expand All @@ -168,6 +169,7 @@ func copyContents(src string, dst string, mode os.FileMode) error {
return fmt.Errorf("failed to get the source file %s mode: %w", src, err)
}
}
// #nosec G304
dstF, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fMode)
if err != nil {
return fmt.Errorf("failed to open the destination file %s: %w", dst, err)
Expand Down Expand Up @@ -217,7 +219,7 @@ func checkDstDir(dst string) (func(e error) error, error) {
if err != nil {
return nil, fmt.Errorf("failed to get the list of dirs to create: %w", err)
}
if err = os.MkdirAll(dstDir, 0o755); err != nil {
if err = os.MkdirAll(dstDir, 0o750); err != nil {
return nil, fmt.Errorf("failed to create the destination file directory %s: %w", dstDir, err)
}
return func(e error) error {
Expand Down
2 changes: 2 additions & 0 deletions internal/app/api/server/private/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,10 @@ func getBaseQueryForItem(db *gorm.DB, option string) *gorm.SqlExpr {
SubQuery()
}

//nolint:typecheck
func validOptions(c *gin.Context, value interface{}) bool {
var vlist []models.IValid
//TODO: Check pointers in switch
switch tvalue := value.(type) {
case *[]models.OptionsActions:
for _, v := range *tvalue {
Expand Down
10 changes: 9 additions & 1 deletion internal/app/api/server/proto/vm/abh_calculator.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"sync"

"soldr/internal/hardening/luavm/vm"

"github.com/sirupsen/logrus"
)

type abhCalculator struct {
Expand All @@ -33,11 +35,17 @@ func calculateABH() (vm.ABH, error) {
if err != nil {
return nil, fmt.Errorf("failed to get the current executable path: %w", err)
}
// #nosec G304
f, err := os.Open(execFile)
if err != nil {
return nil, fmt.Errorf("failed to open the executable file %s: %w", execFile, err)
}
defer f.Close()
defer func(f *os.File) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add G307 into global excludes here https://github.com/vxcontrol/soldr/blob/master/.golangci.yml#L68

Such wrappers everywhere in codebase is redundant and non-profitable. I will not add this comment in every place with the same pattern, please consider this comment for all files.

err = f.Close()
if err != nil {
logrus.Errorf("failed to close file: %s", err)
}
}(f)
h := sha256.New()
if _, err := io.Copy(h, f); err != nil {
return nil, fmt.Errorf("failed to get the hash of the executable file: %w", err)
Expand Down
6 changes: 5 additions & 1 deletion internal/app/api/server/proto/vm/vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ func NewVM(

func (v *VM) ProcessConnectionChallengeRequest(ctx context.Context, req []byte) ([]byte, error) {
var connChallengeReq protoagent.ConnectionChallengeRequest
if err := protoagent.UnpackProtoMessage(&connChallengeReq, req, protoagent.Message_CONNECTION_CHALLENGE_REQUEST); err != nil {
if err := protoagent.UnpackProtoMessage(
&connChallengeReq,
req,
protoagent.Message_CONNECTION_CHALLENGE_REQUEST,
); err != nil {
return nil, fmt.Errorf("failed to unpack the connection challenge request: %w", err)
}
agentID, err := getAgentID(ctx)
Expand Down
16 changes: 8 additions & 8 deletions internal/app/api/utils/meter/gorm.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,31 +93,31 @@ func ApplyGorm(db *gorm.DB) {
recordStartTime(scope)
})
db.Callback().Create().After("gorm:create").Register("metric:create-after", func(scope *gorm.Scope) {
labels := append(labels, makeQueryLabel(scope))
recordHistogram(scope, createHist, labels...)
l := append(labels, makeQueryLabel(scope))
recordHistogram(scope, createHist, l...)
})

db.Callback().Update().Before("gorm:update").Register("metric:update-before", func(scope *gorm.Scope) {
recordStartTime(scope)
})
db.Callback().Update().After("gorm:update").Register("metric:update-after", func(scope *gorm.Scope) {
labels := append(labels, makeQueryLabel(scope))
recordHistogram(scope, updateHist, labels...)
l := append(labels, makeQueryLabel(scope))
recordHistogram(scope, updateHist, l...)
})

db.Callback().Query().Before("gorm:query").Register("metric:query-before", func(scope *gorm.Scope) {
recordStartTime(scope)
})
db.Callback().Query().After("gorm:query").Register("metric:query-after", func(scope *gorm.Scope) {
labels := append(labels, makeQueryLabel(scope))
recordHistogram(scope, queryHist, labels...)
l := append(labels, makeQueryLabel(scope))
recordHistogram(scope, queryHist, l...)
})

db.Callback().Delete().Before("gorm:delete").Register("metric:delete-before", func(scope *gorm.Scope) {
recordStartTime(scope)
})
db.Callback().Delete().After("gorm:delete").Register("metric:delete-after", func(scope *gorm.Scope) {
labels := append(labels, makeQueryLabel(scope))
recordHistogram(scope, deleteHist, labels...)
l := append(labels, makeQueryLabel(scope))
recordHistogram(scope, deleteHist, l...)
})
}
6 changes: 5 additions & 1 deletion internal/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package app

import (
"github.com/oklog/run"
"github.com/sirupsen/logrus"
Copy link
Contributor

@agalitsyn agalitsyn Dec 6, 2022

Choose a reason for hiding this comment

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

Please do not use direct logrus imports, because we have app level router configured using app config https://github.com/vxcontrol/soldr/blob/master/cmd/api/main.go#L153 and package internal/log which should be use for dependency.

Note that not all parts of code uses it now, but it will in future.

)

type AppGroup struct {
Expand All @@ -13,7 +14,10 @@ func NewAppGroup() *AppGroup {
}

func (a *AppGroup) Run() {
a.runGroup.Run()
err := a.runGroup.Run()
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add logging here and in other places just for suppressing linter messages. Logging is a part of public application output. All these new log messages for errors is not useful information about how app runs.

Let's return err if this is possible, or ignore like _ = a.runGroup.Run(), or ignore with comment for linter, and make there places visible and marked for refactoring. In time all such places will be substituted with proper error handling.

if err != nil {
logrus.Errorf("failed to run appGroup: %s", err)
}
}

// Add (function) to the application group. Each actor must be pre-emptable by an
Expand Down
10 changes: 6 additions & 4 deletions internal/app/server/certs/provider/static/static.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package static

//TODO: io/ioutil is deprecated, replace to fs.FS and delete "nolint:staticcheck"
import (
"crypto/ed25519"
"crypto/rand"
Expand Down Expand Up @@ -80,8 +82,8 @@ func getVXCAs(certsDir string) (map[string]*x509Cert, error) {
if filepath.Ext(fi.Name()) != ".cert" {
continue
}
fPath := filepath.Join(vxcasPath, fi.Name())
data, err := os.ReadFile(fPath)
// #nosec G304
data, err := os.ReadFile(filepath.Join(vxcasPath, fi.Name()))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -154,8 +156,8 @@ func getCertKeyFiles(filesInfo []fs.FileInfo, baseDir string) (certKeyFiles, err
if ext != certExt && ext != keyExt {
continue
}
fPath := filepath.Join(baseDir, fi.Name())
data, err := os.ReadFile(fPath)
// #nosec G304
data, err := os.ReadFile(filepath.Join(baseDir, fi.Name()))
if err != nil {
return nil, err
}
Expand Down
3 changes: 3 additions & 0 deletions internal/app/server/config/config.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package config

//TODO: io/ioutil is deprecated, replace to fs.FS and delete "nolint:staticcheck"
import (
"encoding/json"
"errors"
Expand Down Expand Up @@ -133,6 +135,7 @@ var defaultConfig = &Config{

func parseConfigFile(path string) (*Config, error) {
var cfg Config
// #nosec G304
cfgData, err := ioutil.ReadFile(path)
if err != nil {
return nil, err
Expand Down
63 changes: 59 additions & 4 deletions internal/controller/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,62 @@ const (

// List of SQL queries string
const (
sLoadModulesSQL string = "SELECT m.`id`, IFNULL(g.`hash`, '') AS `group_id`, IFNULL(p.`hash`, '') AS `policy_id`, m.`info`, m.`last_update`, m.`last_module_update`, m.`state`, m.`template` FROM `modules` m LEFT JOIN (SELECT * FROM `policies` UNION SELECT 0, '', '{}', NOW(), NOW(), NULL) p ON m.`policy_id` = p.`id` AND p.deleted_at IS NULL LEFT JOIN `groups_to_policies` gp ON gp.`policy_id` = p.`id` LEFT JOIN (SELECT * FROM `groups` UNION SELECT 0, '', '{}', NOW(), NOW(), NULL) g ON gp.`group_id` = g.`id` AND g.deleted_at IS NULL WHERE m.`status` = 'joined' AND NOT (ISNULL(g.`hash`) AND p.`hash` NOT LIKE '') AND m.deleted_at IS NULL"
sGetModuleFieldSQL string = "SELECT `%s` FROM `modules` WHERE `id` = ? LIMIT 1"
sSetModuleFieldSQL string = "UPDATE `modules` SET `%s` = ? WHERE `id` = ?"
sLoadModulesSQL string = `
SELECT
m.id
, IFNULL(g.hash, '') AS group_id
Copy link
Contributor

@agalitsyn agalitsyn Dec 6, 2022

Choose a reason for hiding this comment

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

We can just ignore lll for this const. really weird formatting here, with , in the beginning of row

Copy link
Author

Choose a reason for hiding this comment

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

This technique is necessary in order not to forget the comma when adding. Similar to how Go puts a comma at the end of a line.

Copy link
Author

@BurovAV BurovAV Dec 6, 2022

Choose a reason for hiding this comment

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

Here, writing in one line makes the constant absolutely unreadable for the developer.

, IFNULL(p.hash, '') AS policy_id
, m.info
, m.last_update
, m.last_module_update
, m.state
, m.template
FROM modules m
LEFT JOIN (
SELECT *
FROM policies
UNION
SELECT
0
, ''
, '{}'
, NOW()
, NOW()
, NULL
) p ON
m.policy_id = p.id
AND p.deleted_at IS NULL
LEFT JOIN groups_to_policies gp
ON gp.policy_id = p.id
LEFT JOIN (
SELECT *
FROM groups
UNION
SELECT
0
, ''
, '{}'
, NOW()
, NOW()
, NULL
) g ON
gp.group_id = g.id
AND g.deleted_at IS NULL
WHERE m.status = 'joined'
AND NOT (ISNULL(g.hash)
AND p.hash NOT LIKE '')
AND m.deleted_at IS NULL`

sGetModuleFieldSQL string = `
SELECT %s
FROM modules
WHERE id = ?
LIMIT 1`

sSetModuleFieldSQL string = `
UPDATE modules
SET %s = ?
WHERE id = ?`
)

// tFilesLoaderType is type for loading module
Expand Down Expand Up @@ -115,7 +168,9 @@ func (cl *configLoaderDB) load() ([]*loader.ModuleConfig, error) {
return nil, fmt.Errorf("failed to parse the module config: %w", err)
}
} else {
return nil, fmt.Errorf("failed to load the module config: returned rows do not contain the field '%s'", moduleInfoField)
return nil, fmt.Errorf(
"failed to load the module config: returned rows do not contain the field '%s'", moduleInfoField,
)
}
if groupID, ok := m["group_id"]; ok {
mc.GroupID = groupID
Expand Down
7 changes: 4 additions & 3 deletions internal/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ package errors
import "errors"

var (
ErrConnectionInitializationRequired = errors.New("failed to connect to the server: a connection initialization required")
ErrUnexpectedUnpackType = errors.New("unexpected agent message type")
ErrRecordNotFound = errors.New("record not found")
ErrConnectionInitializationRequired = errors.New("failed to connect to the server: " +
"a connection initialization required")
ErrUnexpectedUnpackType = errors.New("unexpected agent message type")
ErrRecordNotFound = errors.New("record not found")
)
2 changes: 2 additions & 0 deletions internal/hardening/luavm/store/simple/simple.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:staticcheck
package simple

//TODO: io/ioutil is deprecated, replace to fs.FS and delete "nolint:staticcheck"
import (
"encoding/json"
"errors"
Expand Down
10 changes: 9 additions & 1 deletion internal/hardening/luavm/vm/abh_calculator.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"io"
"os"
"path/filepath"

"github.com/sirupsen/logrus"
)

type ABH []byte
Expand All @@ -25,11 +27,17 @@ func (c *abhCalculator) GetABH() (ABH, error) {
if err != nil {
return nil, fmt.Errorf("failed to get the current executable path: %w", err)
}
// #nosec G304
f, err := os.Open(execFile)
if err != nil {
return nil, fmt.Errorf("failed to open the executable file %s: %w", execFile, err)
}
defer f.Close()
defer func(f *os.File) {
err = f.Close()
if err != nil {
logrus.Errorf("failed close file: %s", err)
}
}(f)
h := sha256.New()
if _, err := io.Copy(h, f); err != nil {
return nil, fmt.Errorf("failed to get the hash of the executable file: %w", err)
Expand Down
2 changes: 2 additions & 0 deletions internal/hardening/luavm/vm/securestore.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//nolint:gosec
package vm

//TODO: replace mean cryptographic primitive and delete "nolint:gosec"
import (
"crypto/rc4"
"crypto/sha256"
Expand Down
Loading